Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2748013004:
CustomTabs: Base version for prerender switch (Closed)
DescriptionIntroducing a prerender switch that allows to toggle between regular
prerendering and other modes (hidden tab, ...)
Review-Url: https://codereview.chromium.org/2748013004
Cr-Commit-Position: refs/heads/master@{#460063}
Committed: https://chromium.googlesource.com/chromium/src/+/40ccdb1b22185c9b07ddf1702c52afd4ffe0412e
BUG=708173
Patch Set 1 : Base version #
Total comments: 10
Patch Set 2 : Normalizing names #Patch Set 3 : Rework to stateless switch #Patch Set 4 : Small fixes #
Total comments: 13
Patch Set 5 : Reworked after feedback round 2 #
Total comments: 20
Patch Set 6 : Reworked after feedback round 3 #
Total comments: 16
Patch Set 7 : Some more misc fixes #
Total comments: 2
Patch Set 8 : Fixing scattered logic #
Total comments: 6
Patch Set 9 : Tiny rework of startSpeculation #
Messages
Total messages: 69 (48 generated)
The CQ bit was checked by ahemery@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #1 (id:1) has been deleted
ahemery@chromium.org changed reviewers: + lizeb@chromium.org, mattcarry@chromium.org
https://codereview.chromium.org/2748013004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (left): https://codereview.chromium.org/2748013004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:926: // Ignores mayPrerender() for an empty URL, since it cancels an existing prerender. This is already done in maylaunchUrl and it makes more sense to be done there. Since the function is private, we are sure no other unexpected callers relying on this behaviour exist. https://codereview.chromium.org/2748013004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:950: Pair<WebContents, WebContents> webContentsPair = mExternalPrerenderHandler.addPrerender( Auto formatting https://codereview.chromium.org/2748013004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:960: mSpeculation = SpeculationParams.forPrerender( Auto formatting https://codereview.chromium.org/2748013004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2748013004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:95: static final int DEFAULT_PRERENDER = 0; Not sure of the name https://codereview.chromium.org/2748013004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:911: void cancelPrerender(CustomTabsSessionToken session) { Generic function to cancel the prerendering of a URL https://codereview.chromium.org/2748013004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:922: private void cancelWebContentsPrerender(CustomTabsSessionToken session) { Version targeting what we know typically as "prerender". Running out of names here... https://codereview.chromium.org/2748013004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:932: private boolean startPrerender( Generic function to start prerendering a URL https://codereview.chromium.org/2748013004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:1010: void setPrerenderMode(int mode) { The new switch that enables toggling between usual prerender and other modes (e.g. hidden tab). The clear is done to avoid mixed states. https://codereview.chromium.org/2748013004/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java (right): https://codereview.chromium.org/2748013004/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:1488: // Forcing no prerendering implies falling back to simply creating a spare WebContent Unrelated comment to help readability https://codereview.chromium.org/2748013004/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:1642: tab, "visibility", ignoreFragments ? getBaseExpectedVisibility() : "visible"); Since prerender visibility is prerender and hidden tab is hidden I introduced a function to deal with this transparently in tests.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Base version for prerender switch BUG= ========== to ========== Base version for prerender switch BUG= ==========
ahemery@chromium.org changed reviewers: + mattcary@chromium.org - mattcarry@chromium.org
Description was changed from ========== Base version for prerender switch BUG= ========== to ========== Introducing a prerender switch that allows to toggle between regular prerendering and other modes (hidden tab, ...) BUG= ==========
Thanks! A general comment: there is already a way of setting the prerender "mode", using the "DEBUG_OVERRIDE". If the aim here is purely testing, then you can probably reuse the same mechanism. In this case though, this needs to be guarded to make sure that only "self" can only use it (look in isCallerForegroundOrSelf()). Otherwise, then I would prefer for things to be integrated into SpeculationParams, and more importantly, the setting needs to be per-session. Otherwise an app "A" can set a mode, and cause app "B" to use the same one. Which is clearly not the right behavior here.
The CQ bit was checked by ahemery@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by ahemery@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ahemery@google.com changed reviewers: + ahemery@google.com
Updated to reflect Benoit's feedback https://codereview.chromium.org/2748013004/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (left): https://codereview.chromium.org/2748013004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:927: if (!mayPrerender(session) && !TextUtils.isEmpty(url)) return false; Unnecessary duplicate. https://codereview.chromium.org/2748013004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:931: if (TextUtils.isEmpty(url)) return false; Extracted because behavior is more general than just prerender. See comment line 336 of new version. https://codereview.chromium.org/2748013004/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2748013004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:100: public final int debugOverrideValue; Replaces prefetchOnly to have a record of what the speculation was in a more general sense. https://codereview.chromium.org/2748013004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:107: static SpeculationParams forPrefetch(CustomTabsSessionToken session, String url) { Just reordered. https://codereview.chromium.org/2748013004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:336: // Last one wins and cancels the previous prerender. This was moved from inside the prerenderUrl() function. It is private so we are in control of where this is used and we should have no problem. Factoring behavior common for both prerender and hidden tab. https://codereview.chromium.org/2748013004/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java (right): https://codereview.chromium.org/2748013004/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:145: private int mLatestDebugOverrideValue; This is some sort of state that we use here only. We need this because we sometimes need to have information about the speculation even after the speculationParams have been destroyed. In particular, even when visible, a prerendered document has a prerender visibility property in javascript. https://codereview.chromium.org/2748013004/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:169: mDefaultDebugOverrideValue = CustomTabsConnection.NO_OVERRIDE; The switch that can be acted upon to modify the entire test suite. https://codereview.chromium.org/2748013004/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:1648: ElementContentCriteria initialVisibilityCriteria = new ElementContentCriteria(tab, Unrelated change but used only inside the wait scope. https://codereview.chromium.org/2748013004/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:2141: if (connection.mSpeculation.debugOverrideValue == CustomTabsConnection.NO_OVERRIDE) { We are sure this exist so we can use this value here. https://codereview.chromium.org/2748013004/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:2240: private boolean mayLaunchUrlForTesting(CustomTabsConnection connection, A wrapper that allows us to do some additional instrumentation / routing when there will be the hidden tab path (the test version needs instrumentation to record completion, etc.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2748013004/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2748013004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:100: public final int debugOverrideValue; I would prefer a name like "mode", or something along these lines. Could take the values of PRERENDER, PREFETCH_ONLY, and more soon. PREFETCH_ONLY would be controlled by the value of DEBUG_OVERRIDE, and hidden tab by the per-session setting, or when Chrome calls itself (from the tests). https://codereview.chromium.org/2748013004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:107: static SpeculationParams forPrefetch(CustomTabsSessionToken session, String url) { On 2017/03/21 15:15:58, ahemery1 wrote: > Just reordered. why? Not an issue, but why reordering code? https://codereview.chromium.org/2748013004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:336: // Last one wins and cancels the previous prerender. On 2017/03/21 15:15:57, ahemery1 wrote: > This was moved from inside the prerenderUrl() function. It is private so we are > in control of where this is used and we should have no problem. Factoring > behavior common for both prerender and hidden tab. nit: Is it possible to isolate the changes between the various CLs? Makes it easier to review and understand. Not a big issue here.
The CQ bit was checked by ahemery@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Updated with the following changes : - The speculationMode setting is back but is now stored in the ClientManager per session. - After discussion with Benoit moved from a universal test switch to duplication of tests while factoring the behavior for easy maintenance.
Thanks! A few comments, nothing major. Haven't finished to look at the tests though. https://codereview.chromium.org/2748013004/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (left): https://codereview.chromium.org/2748013004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:931: if (TextUtils.isEmpty(url)) return false; Was this check redundant? https://codereview.chromium.org/2748013004/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2748013004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:596: if (mSpeculation.speculationMode == SpeculationParams.PREFETCH) { Basically the speculation is stopped in two different places depending on the current mode. Isn't it what cancelSpeculation should take care of? https://codereview.chromium.org/2748013004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:681: @VisibleForTesting This is not strictly true that this is VisibleForTesting. I think that these days VisibleForTesting a no-op, so can you remove it? https://codereview.chromium.org/2748013004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:920: void cancelSpeculation(CustomTabsSessionToken session) { nit: Please keep the assertOnUiThread() here. https://codereview.chromium.org/2748013004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:921: if (mSpeculation == null) return; nit: Why breaking up the condition here? https://codereview.chromium.org/2748013004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:922: if (session != null && !session.equals(mSpeculation.session)) return; tiny nit: This reads a bit weird, what about: if (session == null || !session.equals(... ? https://codereview.chromium.org/2748013004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:937: mSpeculation = null; All current paths will set mSpeculation to null, can you move this to cancelSpeculation()? https://codereview.chromium.org/2748013004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:942: switch (getSpeculationModeForSession(session)) { ditto, why isn't prefetch handled here as well? https://codereview.chromium.org/2748013004/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java (right): https://codereview.chromium.org/2748013004/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:1392: final CustomTabsSessionToken token = tiny nit: unnecessary final. https://codereview.chromium.org/2748013004/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:1497: // Forcing no prerendering implies falling back to simply creating a spare WebContent tiny nit: WebContents.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ahemery@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2748013004/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (left): https://codereview.chromium.org/2748013004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:931: if (TextUtils.isEmpty(url)) return false; On 2017/03/23 18:28:28, Benoit L wrote: > Was this check redundant? It was indeed redundant. Already checked in highConfidenceMayLaunchUrl() line 317-320 https://codereview.chromium.org/2748013004/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2748013004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:596: if (mSpeculation.speculationMode == SpeculationParams.PREFETCH) { On 2017/03/23 18:28:29, Benoit L wrote: > Basically the speculation is stopped in two different places depending on the > current mode. Isn't it what cancelSpeculation should take care of? You're absolutely right, missed it. https://codereview.chromium.org/2748013004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:681: @VisibleForTesting On 2017/03/23 18:28:29, Benoit L wrote: > This is not strictly true that this is VisibleForTesting. I think that these > days VisibleForTesting a no-op, so can you remove it? Changed to public. https://codereview.chromium.org/2748013004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:920: void cancelSpeculation(CustomTabsSessionToken session) { On 2017/03/23 18:28:28, Benoit L wrote: > nit: Please keep the assertOnUiThread() here. Moved from subfunction to here. https://codereview.chromium.org/2748013004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:921: if (mSpeculation == null) return; On 2017/03/23 18:28:29, Benoit L wrote: > nit: Why breaking up the condition here? Felt like it had two different functions. First line checking that the cancel is not redundant, and second line checking that the target is correct. Wouldn't mind changing it however as it took me 5 minutes to write this explanation. https://codereview.chromium.org/2748013004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:922: if (session != null && !session.equals(mSpeculation.session)) return; On 2017/03/23 18:28:29, Benoit L wrote: > tiny nit: This reads a bit weird, what about: > > if (session == null || !session.equals(... > > ? Done. https://codereview.chromium.org/2748013004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:937: mSpeculation = null; On 2017/03/23 18:28:29, Benoit L wrote: > All current paths will set mSpeculation to null, can you move this to > cancelSpeculation()? Done. https://codereview.chromium.org/2748013004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:942: switch (getSpeculationModeForSession(session)) { On 2017/03/23 18:28:29, Benoit L wrote: > ditto, why isn't prefetch handled here as well? I included prefetch in the same process, but doing so I had to move stuff around a bit in highConfidenceMayLaunchUrl. Let me know if you think it is worth it. https://codereview.chromium.org/2748013004/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java (right): https://codereview.chromium.org/2748013004/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:1392: final CustomTabsSessionToken token = On 2017/03/23 18:28:29, Benoit L wrote: > tiny nit: unnecessary final. Done. https://codereview.chromium.org/2748013004/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:1497: // Forcing no prerendering implies falling back to simply creating a spare WebContent On 2017/03/23 18:28:29, Benoit L wrote: > tiny nit: WebContents. Done.
https://codereview.chromium.org/2748013004/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2748013004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:320: private void highConfidenceMayLaunchUrl(CustomTabsSessionToken session, Main rework for this patch is here, integrating prefetch into the now common workflow
https://codereview.chromium.org/2748013004/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2748013004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:344: if (temporarySpeculationMode != SpeculationParams.PREFETCH || !didStartSpeculation) { Shouldn't some of this logic go into startSpeculation? I'm concerned that the logic handling is split up between two different places. It makes it difficult to figure out what actually happens for a particular mode.
Thanks, that's clearer this way indeed. A couple more comments. https://codereview.chromium.org/2748013004/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java (right): https://codereview.chromium.org/2748013004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:464: public synchronized int getSpeculationModeForSession(CustomTabsSessionToken session) { tiny nit: I'm usually all for not adding javadoc to simple accessor, but in this case can you document that the default returned value is PRERENDER? https://codereview.chromium.org/2748013004/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2748013004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:337: setSpeculationModeForSession(session, temporarySpeculationMode); That's only a matter of preference, but I don't really like temporarily modifying global state. Since the modification is scopped, can you pass the value to the methods called here? Also, you could then do: int mode = getSpeculationMode(session, overrideValue); https://codereview.chromium.org/2748013004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:683: public void setSpeculationModeForSession(CustomTabsSessionToken session, int speculationMode) { nit: For consistency with the setters / getters above, make these methods package private. https://codereview.chromium.org/2748013004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:920: public void cancelSpeculation(CustomTabsSessionToken session) { nit: Please keep this method package private. https://codereview.chromium.org/2748013004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:938: private void cancelPrerender(CustomTabsSessionToken session) { These two cancel*() methods are private, trivial and used only once. Can you inline them above? https://codereview.chromium.org/2748013004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:949: @VisibleForTesting This method doesn't seem to be called in tests, why is it @VisibleForTesting then? Also, if it's not used in test, please keep it private.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by ahemery@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2748013004/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java (right): https://codereview.chromium.org/2748013004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:464: public synchronized int getSpeculationModeForSession(CustomTabsSessionToken session) { On 2017/03/27 13:18:14, Benoit L wrote: > tiny nit: I'm usually all for not adding javadoc to simple accessor, but in this > case can you document that the default returned value is PRERENDER? Done. https://codereview.chromium.org/2748013004/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2748013004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:337: setSpeculationModeForSession(session, temporarySpeculationMode); On 2017/03/27 13:18:15, Benoit L wrote: > That's only a matter of preference, but I don't really like temporarily > modifying global state. > Since the modification is scopped, can you pass the value to the methods called > here? > > Also, you could then do: > > int mode = getSpeculationMode(session, overrideValue); Done. https://codereview.chromium.org/2748013004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:344: if (temporarySpeculationMode != SpeculationParams.PREFETCH || !didStartSpeculation) { On 2017/03/27 13:11:35, mattcary wrote: > Shouldn't some of this logic go into startSpeculation? > > I'm concerned that the logic handling is split up between two different places. > It makes it difficult to figure out what actually happens for a particular mode. I hear you, however not sure about this one since it seems to me that the logic is higher level. As in, done if the main speculation has failed or if we can still do some other optimizations. I have added a comment to clarify the role of this code. What's your opinion on that lizeb@ ? https://codereview.chromium.org/2748013004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:683: public void setSpeculationModeForSession(CustomTabsSessionToken session, int speculationMode) { On 2017/03/27 13:18:15, Benoit L wrote: > nit: For consistency with the setters / getters above, make these methods > package private. Done. https://codereview.chromium.org/2748013004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:938: private void cancelPrerender(CustomTabsSessionToken session) { On 2017/03/27 13:18:14, Benoit L wrote: > These two cancel*() methods are private, trivial and used only once. > Can you inline them above? Done. https://codereview.chromium.org/2748013004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:949: @VisibleForTesting On 2017/03/27 13:18:15, Benoit L wrote: > This method doesn't seem to be called in tests, why is it @VisibleForTesting > then? > Also, if it's not used in test, please keep it private. Done.
https://codereview.chromium.org/2748013004/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2748013004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:344: if (temporarySpeculationMode != SpeculationParams.PREFETCH || !didStartSpeculation) { On 2017/03/27 14:47:33, ahemery1 wrote: > On 2017/03/27 13:11:35, mattcary wrote: > > Shouldn't some of this logic go into startSpeculation? > > > > I'm concerned that the logic handling is split up between two different > places. > > It makes it difficult to figure out what actually happens for a particular > mode. > > I hear you, however not sure about this one since it seems to me that the logic > is higher level. As in, done if the main speculation has failed or if we can > still do some other optimizations. I have added a comment to clarify the role of > this code. What's your opinion on that lizeb@ ? I was thinking of just pushing this all down into startSpeculation, which would then read something like switch (speculationMode) { case PREFETCH: launched = PrefetchUrl(); preconnectUrls(...) if (!launched) createSpareWebContents() break; etc } There's maybe a couple of duplicated lines but the general flow is comprehensible, and there's less coupling of state across functions (eg, didStartSpeculation) and complicated logic (temporaryMode != PREFETCH || !didStart) which obscures the fairly straightforward idea of what we want to do for each mode. lizeb@ may have other opinions, of course.
https://codereview.chromium.org/2748013004/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://codereview.chromium.org/2748013004/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:396: // If extra headers have been passed, cancel any current prerender, as Note that this applies to prerender only, per the comment. Even though in practice it shouldn't make a big difference. https://codereview.chromium.org/2748013004/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2748013004/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:337: // Smaller optimizations we can do apart from the main speculation. I agree with mattcary@, actually :-)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ahemery@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2748013004/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2748013004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:344: if (temporarySpeculationMode != SpeculationParams.PREFETCH || !didStartSpeculation) { On 2017/03/27 15:10:13, mattcary wrote: > On 2017/03/27 14:47:33, ahemery1 wrote: > > On 2017/03/27 13:11:35, mattcary wrote: > > > Shouldn't some of this logic go into startSpeculation? > > > > > > I'm concerned that the logic handling is split up between two different > > places. > > > It makes it difficult to figure out what actually happens for a particular > > mode. > > > > I hear you, however not sure about this one since it seems to me that the > logic > > is higher level. As in, done if the main speculation has failed or if we can > > still do some other optimizations. I have added a comment to clarify the role > of > > this code. What's your opinion on that lizeb@ ? > > I was thinking of just pushing this all down into startSpeculation, which would > then read something like > > switch (speculationMode) { > case PREFETCH: > launched = PrefetchUrl(); > preconnectUrls(...) > if (!launched) createSpareWebContents() > break; > etc > } > > There's maybe a couple of duplicated lines but the general flow is > comprehensible, and there's less coupling of state across functions (eg, > didStartSpeculation) and complicated logic (temporaryMode != PREFETCH || > !didStart) which obscures the fairly straightforward idea of what we want to do > for each mode. > > lizeb@ may have other opinions, of course. Fair enough, I have modified it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2748013004/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2748013004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:933: case SpeculationParams.PREFETCH: Just reordered to match constants ordering.
Thanks! Only small things remaining. About the commit message: please prefix it with a "tag", in this case "customtabs:", stick to <80 chars lines, and remove the BUG= label since there is no associated bug. Since the CL has been uploaded already, "git cl description" is the command. https://codereview.chromium.org/2748013004/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2748013004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:930: WarmupManager warmupManager = WarmupManager.getInstance(); I think it would be slightly clearer this way: boolean preconnect = true, createSpareWebContents = true; and in the switch: case PREFETCH: preconnect = !didPrefetch; case PRERENDER: createSpareWebContents = !didPrerender; And after the switch: if (preconnect)_warmupManager.preconnect(); if (createSpareWebContents) warmupManager.createSpareWebContents(); https://codereview.chromium.org/2748013004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:999: private boolean prefetchUrl(CustomTabsSessionToken session, String url) { nit: Actually, if we remove the profile construction, this methods is two lines. Inline above?
Description was changed from ========== Introducing a prerender switch that allows to toggle between regular prerendering and other modes (hidden tab, ...) BUG= ========== to ========== Introducing a prerender switch that allows to toggle between regular prerendering and other modes (hidden tab, ...) ==========
lgtm https://codereview.chromium.org/2748013004/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2748013004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:928: private void startSpeculation(CustomTabsSessionToken session, String url, int speculationMode, This is much more comprehensible, thanks.
The CQ bit was checked by ahemery@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
https://codereview.chromium.org/2748013004/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java (right): https://codereview.chromium.org/2748013004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:930: WarmupManager warmupManager = WarmupManager.getInstance(); On 2017/03/28 09:45:15, Benoit L wrote: > I think it would be slightly clearer this way: > > boolean preconnect = true, createSpareWebContents = true; > > and in the switch: > > case PREFETCH: > preconnect = !didPrefetch; > case PRERENDER: > createSpareWebContents = !didPrerender; > > And after the switch: > > if (preconnect)_warmupManager.preconnect(); > if (createSpareWebContents) warmupManager.createSpareWebContents(); Done. https://codereview.chromium.org/2748013004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:999: private boolean prefetchUrl(CustomTabsSessionToken session, String url) { On 2017/03/28 09:45:15, Benoit L wrote: > nit: > Actually, if we remove the profile construction, this methods is two lines. > Inline above? Done.
Description was changed from ========== Introducing a prerender switch that allows to toggle between regular prerendering and other modes (hidden tab, ...) BUG= ========== to ========== Introducing a prerender switch that allows to toggle between regular prerendering and other modes (hidden tab, ...) ==========
Description was changed from ========== Introducing a prerender switch that allows to toggle between regular prerendering and other modes (hidden tab, ...) ========== to ========== Introducing a prerender switch that allows to toggle between regular prerendering and other modes (hidden tab, ...) ==========
lgtm, thanks. Please update the commit message before checking the box, see https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list...
The CQ bit was checked by ahemery@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mattcary@chromium.org Link to the patchset: https://codereview.chromium.org/2748013004/#ps260001 (title: "Tiny rework of startSpeculation")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1490702898944280,
"parent_rev": "c787104b181d6ca1356c1f0b82257ed5a62ad633", "commit_rev":
"40ccdb1b22185c9b07ddf1702c52afd4ffe0412e"}
Message was sent while issue was closed.
Description was changed from ========== Introducing a prerender switch that allows to toggle between regular prerendering and other modes (hidden tab, ...) ========== to ========== Introducing a prerender switch that allows to toggle between regular prerendering and other modes (hidden tab, ...) Review-Url: https://codereview.chromium.org/2748013004 Cr-Commit-Position: refs/heads/master@{#460063} Committed: https://chromium.googlesource.com/chromium/src/+/40ccdb1b22185c9b07ddf1702c52... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:260001) as https://chromium.googlesource.com/chromium/src/+/40ccdb1b22185c9b07ddf1702c52...
Message was sent while issue was closed.
Description was changed from ========== Introducing a prerender switch that allows to toggle between regular prerendering and other modes (hidden tab, ...) Review-Url: https://codereview.chromium.org/2748013004 Cr-Commit-Position: refs/heads/master@{#460063} Committed: https://chromium.googlesource.com/chromium/src/+/40ccdb1b22185c9b07ddf1702c52... ========== to ========== Introducing a prerender switch that allows to toggle between regular prerendering and other modes (hidden tab, ...) Review-Url: https://codereview.chromium.org/2748013004 Cr-Commit-Position: refs/heads/master@{#460063} Committed: https://chromium.googlesource.com/chromium/src/+/40ccdb1b22185c9b07ddf1702c52... BUG=708173 ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
