|
|
DescriptionAndroid: Make the spare renderer accessible to all Chrome tabs.
In Custom Tabs, an external application can "warm up" Chrome. This
creates and initializes a spare renderer, and has been found to be a
significant optimization for loading performance.
This patch moves the spare renderer logic out of Custom Tabs, and makes
it accessible and useful for regular navigations in Chrome. Note that
this patch doesn't add new call sites for the spare renderer creation.
BUG=633964
Committed: https://crrev.com/f7ce563033df1df90b314e4f727596c93c2b3d6d
Cr-Commit-Position: refs/heads/master@{#412510}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : Moar tests. #
Total comments: 8
Patch Set 4 : Address comments. #Patch Set 5 : Make FindBugs happy. #Patch Set 6 : Fix a test issue exposed by this CL. #
Total comments: 6
Patch Set 7 : Address comments. #Messages
Total messages: 39 (26 generated)
The CQ bit was checked by lizeb@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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Android: Make the spare renderer accessible to all Chrome tabs. ========== to ========== Android: Make the spare renderer accessible to all Chrome tabs. In Custom Tabs, an external application can "warm up" Chrome. This creates and initializes a spare renderer, and has been found to be a significant optimization for loading performance. This patch moves the spare renderer logic out of Custom Tabs, and makes it accessible and useful for regular navigations in Chrome. Note that this patch doesn't add new call sites for the spare renderer creation. BUG=633964 ==========
lizeb@chromium.org changed reviewers: + yusufo@chromium.org
lgtm with a few nits and a suggestion. https://codereview.chromium.org/2199393002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java (right): https://codereview.chromium.org/2199393002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:229: public WebContents takeSpareWebContents(boolean incognito, boolean initiallyHiden) { initiallyHidden https://codereview.chromium.org/2199393002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:229: public WebContents takeSpareWebContents(boolean incognito, boolean initiallyHiden) { if we will keep returning null for incognito and initiallyHidden, and I imagine we want to contain that logic here, should we have a canTakeSpareWebContents(boolean incognito, boolean initiallyHidden) { if (incognito || initiallyHiden) return false; retruen mSpareWebContents!=null; } this way Tab can just call that and do if (canTakeSpare) { takeSpare } else { createNew } and we can keep take with no params and shorter. https://codereview.chromium.org/2199393002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:237: /** @return Whether a spare renderer is available. */ no need to one line this one.(at least in general we don't) https://codereview.chromium.org/2199393002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://codereview.chromium.org/2199393002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:414: mHasPrerendered = webContents != null; while you are doing this, do you mind cleaning this one up as well, looks like we have mHasPrerendered and mPrerendered as separate boolean states.
lizeb@chromium.org changed reviewers: + mariakhomenko@chromium.org
Thanks! Adding mariakhomenko@ as a reviewer, PTAL. https://codereview.chromium.org/2199393002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java (right): https://codereview.chromium.org/2199393002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:229: public WebContents takeSpareWebContents(boolean incognito, boolean initiallyHiden) { On 2016/08/05 05:32:43, Yusuf wrote: > initiallyHidden Oops, thanks! Done. https://codereview.chromium.org/2199393002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:229: public WebContents takeSpareWebContents(boolean incognito, boolean initiallyHiden) { On 2016/08/05 05:32:43, Yusuf wrote: > if we will keep returning null for incognito and initiallyHidden, and I imagine > we want to contain that logic here, should we have a > canTakeSpareWebContents(boolean incognito, boolean initiallyHidden) { > if (incognito || initiallyHiden) return false; > retruen mSpareWebContents!=null; > } > > this way Tab can just call that and do > if (canTakeSpare) { > takeSpare > } else { > createNew > } > > and we can keep take with no params and shorter. I would prefer to keep this as a single method, because: - It lowers to chances of mi-using it. - We may be able to use a spare renderer, even if (for instance) initiallyHidden is true. Then we will not always return null. wdyt? https://codereview.chromium.org/2199393002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:237: /** @return Whether a spare renderer is available. */ On 2016/08/05 05:32:43, Yusuf wrote: > no need to one line this one.(at least in general we don't) Done. https://codereview.chromium.org/2199393002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java (right): https://codereview.chromium.org/2199393002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:414: mHasPrerendered = webContents != null; On 2016/08/05 05:32:43, Yusuf wrote: > while you are doing this, do you mind cleaning this one up as well, looks like > we have mHasPrerendered and mPrerendered as separate boolean states. These are separate. Added comments in the member declarations to clarify that.
The CQ bit was checked by lizeb@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: 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 lizeb@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by lizeb@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: This issue passed the CQ dry run.
On 2016/08/08 09:48:20, Benoit L wrote: > Thanks! > > Adding mariakhomenko@ as a reviewer, PTAL. > > https://codereview.chromium.org/2199393002/diff/40001/chrome/android/java/src... > File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java > (right): > > https://codereview.chromium.org/2199393002/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:229: > public WebContents takeSpareWebContents(boolean incognito, boolean > initiallyHiden) { > On 2016/08/05 05:32:43, Yusuf wrote: > > initiallyHidden > > Oops, thanks! > Done. > > https://codereview.chromium.org/2199393002/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:229: > public WebContents takeSpareWebContents(boolean incognito, boolean > initiallyHiden) { > On 2016/08/05 05:32:43, Yusuf wrote: > > if we will keep returning null for incognito and initiallyHidden, and I > imagine > > we want to contain that logic here, should we have a > > canTakeSpareWebContents(boolean incognito, boolean initiallyHidden) { > > if (incognito || initiallyHiden) return false; > > retruen mSpareWebContents!=null; > > } > > > > this way Tab can just call that and do > > if (canTakeSpare) { > > takeSpare > > } else { > > createNew > > } > > > > and we can keep take with no params and shorter. > > I would prefer to keep this as a single method, because: > - It lowers to chances of mi-using it. > - We may be able to use a spare renderer, even if (for instance) initiallyHidden > is true. Then we will not always return null. > > wdyt? > > https://codereview.chromium.org/2199393002/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:237: /** > @return Whether a spare renderer is available. */ > On 2016/08/05 05:32:43, Yusuf wrote: > > no need to one line this one.(at least in general we don't) > > Done. > > https://codereview.chromium.org/2199393002/diff/40001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java > (right): > > https://codereview.chromium.org/2199393002/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:414: > mHasPrerendered = webContents != null; > On 2016/08/05 05:32:43, Yusuf wrote: > > while you are doing this, do you mind cleaning this one up as well, looks like > > we have mHasPrerendered and mPrerendered as separate boolean states. > > These are separate. Added comments in the member declarations to clarify that. Sorry for taking a long time to get to this -- was out sick for a couple of days, will do the code review by EOD tomorrow.
lgtm Thanks for adding all the tests! Looks good, I just have a few nits. https://codereview.chromium.org/2199393002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java (right): https://codereview.chromium.org/2199393002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:240: @VisibleForTesting I think you should remove @VisibleForTesting annotation here since you are calling it from customtabs now https://codereview.chromium.org/2199393002/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/WarmupManagerTest.java (right): https://codereview.chromium.org/2199393002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/WarmupManagerTest.java:85: CriteriaHelper.pollUiThread(new Criteria("App menu was not shown") { I think you need to update the comment here https://codereview.chromium.org/2199393002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/WarmupManagerTest.java:101: @UiThreadTest I think a comment to differentiate what you are testing here from the method above would be nice
Done, thanks! https://codereview.chromium.org/2199393002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java (right): https://codereview.chromium.org/2199393002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:240: @VisibleForTesting On 2016/08/11 17:34:37, Maria wrote: > I think you should remove @VisibleForTesting annotation here since you are > calling it from customtabs now Done. https://codereview.chromium.org/2199393002/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/WarmupManagerTest.java (right): https://codereview.chromium.org/2199393002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/WarmupManagerTest.java:85: CriteriaHelper.pollUiThread(new Criteria("App menu was not shown") { On 2016/08/11 17:34:37, Maria wrote: > I think you need to update the comment here Done. https://codereview.chromium.org/2199393002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/WarmupManagerTest.java:101: @UiThreadTest On 2016/08/11 17:34:37, Maria wrote: > I think a comment to differentiate what you are testing here from the method > above would be nice Done.
The CQ bit was checked by lizeb@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: This issue passed the CQ dry run.
The CQ bit was checked by lizeb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org, mariakhomenko@chromium.org Link to the patchset: https://codereview.chromium.org/2199393002/#ps120001 (title: "Address comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Android: Make the spare renderer accessible to all Chrome tabs. In Custom Tabs, an external application can "warm up" Chrome. This creates and initializes a spare renderer, and has been found to be a significant optimization for loading performance. This patch moves the spare renderer logic out of Custom Tabs, and makes it accessible and useful for regular navigations in Chrome. Note that this patch doesn't add new call sites for the spare renderer creation. BUG=633964 ========== to ========== Android: Make the spare renderer accessible to all Chrome tabs. In Custom Tabs, an external application can "warm up" Chrome. This creates and initializes a spare renderer, and has been found to be a significant optimization for loading performance. This patch moves the spare renderer logic out of Custom Tabs, and makes it accessible and useful for regular navigations in Chrome. Note that this patch doesn't add new call sites for the spare renderer creation. BUG=633964 Committed: https://crrev.com/f7ce563033df1df90b314e4f727596c93c2b3d6d Cr-Commit-Position: refs/heads/master@{#412510} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/f7ce563033df1df90b314e4f727596c93c2b3d6d Cr-Commit-Position: refs/heads/master@{#412510}
Message was sent while issue was closed.
WarmupManagerTest#testCreateAndTakeSpareRenderer failed on 3 different bots, I'm reverting
Message was sent while issue was closed.
On 2016/08/17 16:59:26, boliu wrote: > WarmupManagerTest#testCreateAndTakeSpareRenderer failed on 3 different bots, I'm > reverting Hmm, I guess no need for new bug. Here are the bots: https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg... https://build.chromium.org/p/chromium.android/builders/KitKat%20Tablet%20Test... https://build.chromium.org/p/chromium.android/builders/Jelly%20Bean%20Tester/...
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2254863002/ by boliu@chromium.org. The reason for reverting is: New test WarmupManagerTest#testCreateAndTakeSpareRenderer failing on 3 different bots: https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg... https://build.chromium.org/p/chromium.android/builders/KitKat%20Tablet%20Test... https://build.chromium.org/p/chromium.android/builders/Jelly%20Bean%20Tester/....
Message was sent while issue was closed.
it's a java exception turned crash, native part of the stack looks semi-bogus though.. 0210f: 08-17 15:38:34.579 31317 31317 W System.err: java.lang.AssertionError 0210f: 08-17 15:38:34.579 31317 31317 W System.err: at org.chromium.content.browser.webcontents.WebContentsObserverProxy.destroy(WebContentsObserverProxy.java:257) 0210f: 08-17 15:38:34.579 31317 31317 W System.err: at org.chromium.content.browser.webcontents.WebContentsImpl.nativeDestroyWebContents(Native Method) 0210f: 08-17 15:38:34.579 31317 31317 W System.err: at org.chromium.content.browser.webcontents.WebContentsImpl.destroy(WebContentsImpl.java:142) 0210f: 08-17 15:38:34.579 31317 31317 W System.err: at org.chromium.chrome.browser.WarmupManagerTest$6.run(WarmupManagerTest.java:94) 0210f: 08-17 15:38:34.579 31317 31317 W System.err: at android.os.Handler.handleCallback(Handler.java:733) 0210f: 08-17 15:38:34.579 31317 31317 W System.err: at android.os.Handler.dispatchMessage(Handler.java:95) 0210f: 08-17 15:38:34.579 31317 31317 W System.err: at android.os.Looper.loop(Looper.java:136) 0210f: 08-17 15:38:34.579 31317 31317 W System.err: at android.app.ActivityThread.main(ActivityThread.java:5001) 0210f: 08-17 15:38:34.579 31317 31317 W System.err: at java.lang.reflect.Method.invokeNative(Native Method) 0210f: 08-17 15:38:34.579 31317 31317 W System.err: at java.lang.reflect.Method.invoke(Method.java:515) 0210f: 08-17 15:38:34.579 31317 31317 W System.err: at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:785) 0210f: 08-17 15:38:34.579 31317 31317 W System.err: at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:601) 0210f: 08-17 15:38:34.579 31317 31317 W System.err: at dalvik.system.NativeStart.main(Native Method) 0210f: 08-17 15:38:34.599 31317 31337 D dalvikvm: GC_EXPLICIT freed 122K, 5% free 17065K/17932K, paused 2ms+2ms, total 19ms 0210f: 08-17 15:38:34.609 31317 31317 F chromium: [FATAL:jni_android.cc(236)] Please include Java exception stack in crash report 00cc9e39 content::DevToolsProtocolDispatcher::OnBrowserCreateTarget(content::DevToolsCommandId, std::__1::unique_ptr<base::DictionaryValue, std::__1::default_delete<base::DictionaryValue> >)+156 /b/c/b/android/src/out/Debug/gen/content/browser/devtools/protocol/devtools_protocol_dispatcher.cc:1720 v------> base::internal::BindState<void (media::MediaDecoderJob::*)(media::MediaCodecStatus, bool, base::TimeDelta, base::TimeDelta), base::internal::UnretainedWrapper<media::MediaDecoderJob>, media::MediaCodecStatus, bool, base::TimeDelta, base::TimeDelta>::BindState<void (media::MediaDecoderJob::*)(media::MediaCodecStatus, bool, base::TimeDelta, base::TimeDelta), base::internal::UnretainedWrapper<media::MediaDecoderJob>, media::MediaCodecStatus, bool, base::TimeDelta const&, base::TimeDelta const&>(void (media::MediaDecoderJob::*&&)(media::MediaCodecStatus, bool, base::TimeDelta, base::TimeDelta), base::internal::UnretainedWrapper<media::MediaDecoderJob>&&, media::MediaCodecStatus&&, bool&&, base::TimeDelta const&, base::TimeDelta const&) /b/c/b/android/src/base/bind_internal.h:384 v------> base::Callback<base::internal::MakeUnboundRunTypeImpl<void (media::MediaDecoderJob::*)(media::MediaCodecStatus, bool, base::TimeDelta, base::TimeDelta), base::internal::UnretainedWrapper<media::MediaDecoderJob>, media::MediaCodecStatus, bool, base::TimeDelta const&, base::TimeDelta const&>::Type, (base::internal::CopyMode)1> base::Bind<void (media::MediaDecoderJob::*)(media::MediaCodecStatus, bool, base::TimeDelta, base::TimeDelta), base::internal::UnretainedWrapper<media::MediaDecoderJob>, media::MediaCodecStatus, bool, base::TimeDelta const&, base::TimeDelta const&>(void (media::MediaDecoderJob::*&&)(media::MediaCodecStatus, bool, base::TimeDelta, base::TimeDelta), base::internal::UnretainedWrapper<media::MediaDecoderJob>&&, media::MediaCodecStatus&&, bool&&, base::TimeDelta const&, base::TimeDelta const&) /b/c/b/android/src/base/bind.h:38 00c9a821 media::MediaDecoderJob::DecodeCurrentAccessUnit(base::TimeTicks, base::TimeDelta)+284 /b/c/b/android/src/media/base/android/media_decoder_job.cc:358 00c9aaf1 media::MediaDecoderJob::DecodeCurrentAccessUnit(base::TimeTicks, base::TimeDelta)+1004 /b/c/b/android/src/media/base/android/media_decoder_job.cc:375 v------> base::Callback<void (net::NetworkChangeNotifier::ConnectionType), (base::internal::CopyMode)1>::Callback(base::Callback<void (net::NetworkChangeNotifier::ConnectionType), (base::internal::CopyMode)1> const&) /b/c/b/android/src/base/callback.h:350 v------> base::internal::BindState<base::Callback<void (net::NetworkChangeNotifier::ConnectionType), (base::internal::CopyMode)1>, net::NetworkChangeNotifier::ConnectionType>::BindState<base::Callback<void (net::NetworkChangeNotifier::ConnectionType), (base::internal::CopyMode)1>&, net::NetworkChangeNotifier::ConnectionType>(base::Callback<void (net::NetworkChangeNotifier::ConnectionType), (base::internal::CopyMode)1>&, net::NetworkChangeNotifier::ConnectionType&&) /b/c/b/android/src/base/bind_internal.h:384 v------> base::Callback<base::internal::MakeUnboundRunTypeImpl<base::Callback<void (net::NetworkChangeNotifier::ConnectionType), (base::internal::CopyMode)1>&, net::NetworkChangeNotifier::ConnectionType>::Type, (base::internal::CopyMode)1> base::Bind<base::Callback<void (net::NetworkChangeNotifier::ConnectionType), (base::internal::CopyMode)1>&, net::NetworkChangeNotifier::ConnectionType>(base::Callback<void (net::NetworkChangeNotifier::ConnectionType), (base::internal::CopyMode)1>&, net::NetworkChangeNotifier::ConnectionType&&) /b/c/b/android/src/base/bind.h:38 00cd6529 content::BackgroundSyncNetworkObserverAndroid::Observer::NotifyConnectionTypeChanged(_JNIEnv*, base::android::JavaParamRef<_jobject*> const&, int)+140 |