Revert "Delay readability check until page load is done"
This reverts commit c8c46c1c6a917c8febacc96c8bf41e92951388b9.
Reason for revert: Causing a crash with threading and the Profile
supplier
Original change's description:
> Delay readability check until page load is done
>
> 1) Removing the server readability check from isReadable so the MTB and
> app menu won't trigger the readability.
>
> 2) added readability check call 3 seconds after
> didFirstVisuallyNonEmptyPaint
>
> 3) removed other readability checks that happen early on tab selection
>
> Bug: 335272896
> Change-Id: I2527b83ba497acc8d739c1f92ef7d9a6a1dfb336
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5484321
> Commit-Queue: Andrea Gomez <andreaxg@google.com>
> Reviewed-by: Basia Zimirska <basiaz@google.com>
> Auto-Submit: Andrea Gomez <andreaxg@google.com>
> Cr-Commit-Position: refs/heads/main@{#1296915}
Bug: 335272896
Change-Id: Ide5e2bd4c7206fe4bf348ba061970231c739592f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5517858
Reviewed-by: Basia Zimirska <basiaz@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Basia Zimirska <basiaz@google.com>
Auto-Submit: Andrea Gomez <andreaxg@google.com>
Cr-Commit-Position: refs/heads/main@{#1296997}
diff --git a/chrome/browser/readaloud/android/java/src/org/chromium/chrome/browser/readaloud/ReadAloudController.java b/chrome/browser/readaloud/android/java/src/org/chromium/chrome/browser/readaloud/ReadAloudController.java
index 53f955b..103b344 100644
--- a/chrome/browser/readaloud/android/java/src/org/chromium/chrome/browser/readaloud/ReadAloudController.java
+++ b/chrome/browser/readaloud/android/java/src/org/chromium/chrome/browser/readaloud/ReadAloudController.java
@@ -28,8 +28,6 @@
import org.chromium.base.supplier.ObservableSupplier;
import org.chromium.base.supplier.ObservableSupplierImpl;
import org.chromium.base.supplier.OneShotCallback;
-import org.chromium.base.task.PostTask;
-import org.chromium.base.task.TaskTraits;
import org.chromium.chrome.browser.browser_controls.BrowserControlsSizer;
import org.chromium.chrome.browser.device.DeviceConditions;
import org.chromium.chrome.browser.language.AppLocaleUtils;
@@ -86,9 +84,6 @@
private final Activity mActivity;
private final ObservableSupplier<Profile> mProfileSupplier;
private final ObserverList<Runnable> mReadabilityUpdateObserverList = new ObserverList<>();
- // Delay added to readability check that should run it after largest contentful paint for >85%
- // of users http://uma/p/chrome/timeline_v2?sid=c975abf9022aac7b36bf28285f068dd6
- private static final int READABILITY_DELAY = 3000;
private static final int MAX_URL_ENTRIES = 300;
private final LruCache<Integer, Boolean> mReadabilityMap = new LruCache<>(MAX_URL_ENTRIES);
// the key is the url hash, the value is time it was added to the map
@@ -451,6 +446,7 @@
Log.d(TAG, "onUrlUpdated to %s", tab.getUrl().getPossiblyInvalidSpec());
notifyReadabilityMayHaveChanged();
if (tab != null && tab.getUrl() != null && tab.getUrl().isValid()) {
+ maybeCheckReadability(tab.getUrl());
maybeHandleTabReload(tab, tab.getUrl());
maybeStopPlayback(tab);
}
@@ -476,12 +472,14 @@
}
@Override
- public void didFirstVisuallyNonEmptyPaint(Tab tab) {
- if (tab != null) {
- PostTask.postDelayedTask(
- TaskTraits.USER_VISIBLE,
- () -> maybeCheckReadability(tab.getUrl()),
- READABILITY_DELAY);
+ public void onShown(Tab tab, @TabSelectionType int type) {
+ // This method is called when selecting and showing a cached tab (as
+ // opposite to a tab that has to be loaded).
+ Log.d(
+ TAG,
+ "onShown called for " + tab.getUrl().getPossiblyInvalidSpec());
+ if (tab != null && tab.getUrl() != null) {
+ maybeCheckReadability(tab.getUrl());
}
}
@@ -492,6 +490,7 @@
TAG,
"onRestoreCompleted called for "
+ tab.getUrl().getPossiblyInvalidSpec());
+ maybeCheckReadability(tab.getUrl());
}
}
@@ -636,7 +635,6 @@
}
mReadabilityMap.remove(sanitizedUrlHash);
mReadabilityRequestTimeMap.remove(sanitizedUrlHash);
- notifyReadabilityMayHaveChanged();
}
return false;
}
@@ -688,6 +686,7 @@
Boolean isReadable = mReadabilityMap.get(sanitizedUrlHash);
return isReadable == null ? false : isReadable;
}
+ maybeCheckReadability(tab.getUrl());
}
return false;
}
@@ -1030,6 +1029,7 @@
}
if (language == null) {
+ Log.d(TAG, "Neither page nor app language known. Falling back to en.");
language = "en";
}
diff --git a/chrome/browser/readaloud/android/java/src/org/chromium/chrome/browser/readaloud/ReadAloudControllerUnitTest.java b/chrome/browser/readaloud/android/java/src/org/chromium/chrome/browser/readaloud/ReadAloudControllerUnitTest.java
index 9b7a73c9..9224cfdc 100644
--- a/chrome/browser/readaloud/android/java/src/org/chromium/chrome/browser/readaloud/ReadAloudControllerUnitTest.java
+++ b/chrome/browser/readaloud/android/java/src/org/chromium/chrome/browser/readaloud/ReadAloudControllerUnitTest.java
@@ -47,9 +47,6 @@
import org.chromium.base.ApplicationState;
import org.chromium.base.Promise;
import org.chromium.base.supplier.ObservableSupplierImpl;
-import org.chromium.base.task.TaskTraits;
-import org.chromium.base.task.test.ShadowPostTask;
-import org.chromium.base.task.test.ShadowPostTask.TestImpl;
import org.chromium.base.test.BaseRobolectricTestRunner;
import org.chromium.base.test.util.Features;
import org.chromium.base.test.util.Features.DisableFeatures;
@@ -111,7 +108,7 @@
@RunWith(BaseRobolectricTestRunner.class)
@Config(
manifest = Config.NONE,
- shadows = {ShadowDeviceConditions.class, ShadowPostTask.class})
+ shadows = {ShadowDeviceConditions.class})
@EnableFeatures({
ChromeFeatureList.READALOUD,
ChromeFeatureList.READALOUD_PLAYBACK,
@@ -190,14 +187,6 @@
@Before
public void setUp() {
MockitoAnnotations.initMocks(this);
- ShadowPostTask.setTestImpl(
- new TestImpl() {
- @Override
- public void postDelayedTask(
- @TaskTraits int taskTraits, Runnable task, long delay) {
- task.run();
- }
- });
mProfileSupplier = new ObservableSupplierImpl<>();
mProfileSupplier.set(mMockProfile);
doReturn(true).when(mMockProfile).isNativeInitialized();
@@ -363,6 +352,16 @@
}
@Test
+ public void testOnUrlUpdated() {
+ GURL gurl = new GURL("https://en.wikipedia.org/wiki/Alphabet_Inc.");
+ when(mTab.getUrl()).thenReturn(gurl);
+ mController.getTabModelTabObserverforTests().onUrlUpdated(mTab);
+
+ // check readability for new url
+ verify(mHooksImpl).isPageReadable(eq(gurl.getPossiblyInvalidSpec()), any());
+ }
+
+ @Test
public void testOnActivityAttachmentChanged() {
// change tab attachment before any playback starts - tests null checks
mController
@@ -688,8 +687,7 @@
// advance by 1s - we're past the 1h limit, the record should be deleted
mClock.advanceCurrentTimeMillis(1000);
assertFalse(mController.isReadable(mTab));
- // make sure readability isn't called again
- verify(mHooksImpl, times(1))
+ verify(mHooksImpl, times(2))
.isPageReadable(eq(sTestGURL.getSpec()), mCallbackCaptor.capture());
}
@@ -2380,14 +2378,6 @@
verify(mPlayback, never()).seekToWord(0, 8);
}
- @Test
- public void testDidFirstVisuallyNonEmptyPaint() {
- GURL gurl = new GURL("https://en.wikipedia.org/wiki/Alphabet_Inc.");
- when(mTab.getUrl()).thenReturn(gurl);
- mController.getTabModelTabObserverforTests().didFirstVisuallyNonEmptyPaint(mTab);
- verify(mHooksImpl).isPageReadable(eq(gurl.getPossiblyInvalidSpec()), any());
- }
-
private void requestAndStartPlayback() {
mFakeTranslateBridge.setCurrentLanguage("en");
mTab.setGurlOverrideForTesting(new GURL("https://en.wikipedia.org/wiki/Google"));
diff --git a/chrome/browser/readaloud/android/java/src/org/chromium/chrome/browser/readaloud/ReadAloudToolbarButtonController.java b/chrome/browser/readaloud/android/java/src/org/chromium/chrome/browser/readaloud/ReadAloudToolbarButtonController.java
index a19a414..da6ddfe 100644
--- a/chrome/browser/readaloud/android/java/src/org/chromium/chrome/browser/readaloud/ReadAloudToolbarButtonController.java
+++ b/chrome/browser/readaloud/android/java/src/org/chromium/chrome/browser/readaloud/ReadAloudToolbarButtonController.java
@@ -83,6 +83,7 @@
@Override
protected boolean shouldShowButton(Tab tab) {
+
if (!super.shouldShowButton(tab) || tab == null || mControllerSupplier.get() == null) {
return false;
}