Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
3 years, 7 months ago by Bryan McQuade Modified:
3 years, 7 months ago Reviewers:
Avi (use Gerrit), Peter Kasting, Maria, mattcary, boliu, Fady Samuel, dgozman, Sami CC:
cbentzel+watch_chromium.org, chromium-reviews, creis+watch_chromium.org, csharrison+watch_chromium.org, darin-cc_chromium.org, gavinp+prer_chromium.org, jam, loading-reviews+metrics_chromium.org, nasko+codewatch_chromium.org, speed-metrics-reviews_chromium.org, tburkard+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionProvide WebContents::CreateParams to tab helpers.
With browser-side navigation enabled, navigations in new foreground tabs are
considered to have started in the background. This is a change in behavior.
This causes page load metrics to incorrectly consider these page loads to have
started in the background, which skews all of Chrome's page load metrics
(PageLoad.* in UMA).
clamy explains:
"""
the background/foreground issue seems to be due to the ordering of calls in
chrome/browser/ui/browser_navigator.cc when we create a new tab.
What happens is:
- we first create a WebContents and ask it to navigate
(https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_navigator.cc?type=cs&l=557).
This result in the creation of a NavigationHandle, so we fire DidStartNavigation.
In the current architecture, we fire DidStartNavigation after receiving
DidStartProvisionalLoad.
- then we insert the WebContents in the tab strip
(https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_navigator.cc?type=cs&l=589).
This fires WebContentsImpl::WasShown.
"""
This patch implements Nasko's proposed fix to expose the WebContents
CreateParams to interested WebContentsObservers, page load metrics's
MetricsWebContentsObserver being the first consumer.
We don't have the CreateParams in all places where a WebContents is passed to
TabHelpers, so we wrap the CreateParams in a base::Optional<> and provide
CreateParams in cases where they are available.
BUG=725347
Review-Url: https://codereview.chromium.org/2894973002
Cr-Commit-Position: refs/heads/master@{#474895}
Committed: https://chromium.googlesource.com/chromium/src/+/42b72063edb66bfd015bd9dea116c3e3e937d3eb
Patch Set 1 #Patch Set 2 : finish implementation #Patch Set 3 : fix comment #Patch Set 4 : disable dcheck #Patch Set 5 : switch to optional #Patch Set 6 : fix android build #Patch Set 7 : fix initial foreground state #Patch Set 8 : revert behavior changes #Patch Set 9 : fix android build #Patch Set 10 : fix android build #Patch Set 11 : add missing include #Patch Set 12 : fix tests #Patch Set 13 : set prerender WC to initially_hidden #Patch Set 14 : fix prerender issues #Patch Set 15 : improve comment #Patch Set 16 : rebase #
Total comments: 11
Patch Set 17 : rebase #
Created: 3 years, 7 months ago
Messages
Total messages: 87 (66 generated)
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
Description was changed from ========== Partial impl to provide WebContents::CreateParams to tab helpers. BUG= ========== to ========== Provide WebContents::CreateParams to tab helpers. BUG= ==========
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 bmcquade@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_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by bmcquade@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_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by bmcquade@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_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) 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 bmcquade@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 bmcquade@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by bmcquade@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 bmcquade@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_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by bmcquade@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 bmcquade@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 bmcquade@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_...)
The CQ bit was checked by bmcquade@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_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bmcquade@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_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Provide WebContents::CreateParams to tab helpers. BUG= ========== to ========== Provide WebContents::CreateParams to tab helpers. BUG=725347 ==========
The CQ bit was checked by bmcquade@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Description was changed from ========== Provide WebContents::CreateParams to tab helpers. BUG=725347 ========== to ========== Provide WebContents::CreateParams to tab helpers. With browser-side navigation enabled, navigations in new foreground tabs are considered to have started in the background. This is a change in behavior. This causes page load metrics to incorrectly consider these page loads to have started in the background, which skews all of Chrome's page load metrics (PageLoad.* in UMA). clamy explains: """ the background/foreground issue seems to be due to the ordering of calls in chrome/browser/ui/browser_navigator.cc when we create a new tab. What happens is: - we first create a WebContents and ask it to navigate (https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_navigator.cc?t...). This result in the creation of a NavigationHandle, so we fire DidStartNavigation. In the current architecture, we fire DidStartNavigation after receiving DidStartProvisionalLoad. - then we insert the WebContents in the tab strip (https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_navigator.cc?t...). This fires WebContentsImpl::WasShown. """ This patch implements Nasko's proposed fix to expose the WebContents CreateParams to interested WebContentsObservers, page load metrics's MetricsWebContentsObserver being the first consumer. We don't have the CreateParams in all places where a WebContents is passed to TabHelpers, so we wrap the CreateParams in a base::Optional<> and provide CreateParams in cases where they are available. BUG=725347 ==========
Description was changed from ========== Provide WebContents::CreateParams to tab helpers. With browser-side navigation enabled, navigations in new foreground tabs are considered to have started in the background. This is a change in behavior. This causes page load metrics to incorrectly consider these page loads to have started in the background, which skews all of Chrome's page load metrics (PageLoad.* in UMA). clamy explains: """ the background/foreground issue seems to be due to the ordering of calls in chrome/browser/ui/browser_navigator.cc when we create a new tab. What happens is: - we first create a WebContents and ask it to navigate (https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_navigator.cc?t...). This result in the creation of a NavigationHandle, so we fire DidStartNavigation. In the current architecture, we fire DidStartNavigation after receiving DidStartProvisionalLoad. - then we insert the WebContents in the tab strip (https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_navigator.cc?t...). This fires WebContentsImpl::WasShown. """ This patch implements Nasko's proposed fix to expose the WebContents CreateParams to interested WebContentsObservers, page load metrics's MetricsWebContentsObserver being the first consumer. We don't have the CreateParams in all places where a WebContents is passed to TabHelpers, so we wrap the CreateParams in a base::Optional<> and provide CreateParams in cases where they are available. BUG=725347 ========== to ========== Provide WebContents::CreateParams to tab helpers. With browser-side navigation enabled, navigations in new foreground tabs are considered to have started in the background. This is a change in behavior. This causes page load metrics to incorrectly consider these page loads to have started in the background, which skews all of Chrome's page load metrics (PageLoad.* in UMA). clamy explains: """ the background/foreground issue seems to be due to the ordering of calls in chrome/browser/ui/browser_navigator.cc when we create a new tab. What happens is: - we first create a WebContents and ask it to navigate (https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_navigator.cc?t...). This result in the creation of a NavigationHandle, so we fire DidStartNavigation. In the current architecture, we fire DidStartNavigation after receiving DidStartProvisionalLoad. - then we insert the WebContents in the tab strip (https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_navigator.cc?t...). This fires WebContentsImpl::WasShown. """ This patch implements Nasko's proposed fix to expose the WebContents CreateParams to interested WebContentsObservers, page load metrics's MetricsWebContentsObserver being the first consumer. We don't have the CreateParams in all places where a WebContents is passed to TabHelpers, so we wrap the CreateParams in a base::Optional<> and provide CreateParams in cases where they are available. BUG=725347 ==========
The CQ bit was checked by bmcquade@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.
avi@chromium.org changed reviewers: + avi@chromium.org
This looks like a reasonable thing to do. My only concern is that we might want to plumb this more. I called out one particular place, but because attaching WebContents to the tab strip is only done to WebContents that are intended for that, in theory shouldn't we be able to pass in the creation params always? Hmmm... LGTM https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/android... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/android... chrome/browser/android/tab_android.cc:122: TabHelpers::AttachTabHelpers(web_contents, base::nullopt); Is this not susceptible to the same issue? You may need to plumb this further down this call chain... https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:82: prerender::PrerenderContents::FromWebContents(web_contents) != nullptr; prerender 🙄
Thanks! https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/android... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/android... chrome/browser/android/tab_android.cc:122: TabHelpers::AttachTabHelpers(web_contents, base::nullopt); On 2017/05/23 at 15:38:59, Avi (ping after 24h) wrote: > Is this not susceptible to the same issue? You may need to plumb this further down this call chain... This path doesn't appear to be the one that's exercised for the specific bug, but I agree it'd be better to provide the CreateParams here if available. Tracing the call chain, this is called from TabAndroid::InitWebContents, which gets the webcontents via WebContents::FromJavaWebContents (JNI). I wasn't able to figure out where if at all CreateParams are specified for constructing these WebContents, and it doesn't appear that they'd be available here given we've crossed the Java/Native boundary. So, I think we're unfortunately unable to get access to the CreateParams here, as best I can tell. Happy to add support here if there's a way to do it, though. https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:82: prerender::PrerenderContents::FromWebContents(web_contents) != nullptr; On 2017/05/23 at 15:38:59, Avi (ping after 24h) wrote: > prerender 🙄 sorry, what are you requesting here? looks like the code review tool might've eaten part of your comment.
Note to reviewers: this is just a refactor that adds an additional param to some WebContents-related callbacks. It should be straightforward for you to review, thanks! bolio, mariaknomenko, PTAL for android-related files dgozman, PTAL for devtools famuel, PTAL for guest_view mattcary, PTAL for prerender pkasting, PTAL for chrome/browser/ui skyostil, PTAL for headless Thanks!
https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:82: prerender::PrerenderContents::FromWebContents(web_contents) != nullptr; On 2017/05/23 20:29:57, Bryan McQuade wrote: > On 2017/05/23 at 15:38:59, Avi (ping after 24h) wrote: > > prerender 🙄 > > sorry, what are you requesting here? looks like the code review tool might've > eaten part of your comment. No request from me here.
android_webview and components/web_contents_delegate_android rs lgtm I don't own chrome/android code, but probably needs some work. https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/android... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/android... chrome/browser/android/tab_android.cc:122: TabHelpers::AttachTabHelpers(web_contents, base::nullopt); On 2017/05/23 20:29:57, Bryan McQuade wrote: > On 2017/05/23 at 15:38:59, Avi (ping after 24h) wrote: > > Is this not susceptible to the same issue? You may need to plumb this further > down this call chain... > > This path doesn't appear to be the one that's exercised for the specific bug, > but I agree it'd be better to provide the CreateParams here if available. > > Tracing the call chain, this is called from TabAndroid::InitWebContents, which > gets the webcontents via WebContents::FromJavaWebContents (JNI). I wasn't able > to figure out where if at all CreateParams are specified for constructing these > WebContents, and it doesn't appear that they'd be available here given we've > crossed the Java/Native boundary. > > So, I think we're unfortunately unable to get access to the CreateParams here, > as best I can tell. Happy to add support here if there's a way to do it, though. It comes from https://cs.chromium.org/chromium/src/chrome/browser/android/web_contents_fact... I think probably should figure out a way to plumb the CreateParams here if it's needed. UserData maybe?
On 2017/05/23 at 21:16:28, boliu wrote: > android_webview and components/web_contents_delegate_android rs lgtm > > I don't own chrome/android code, but probably needs some work. > > https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/android... > File chrome/browser/android/tab_android.cc (right): > > https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/android... > chrome/browser/android/tab_android.cc:122: TabHelpers::AttachTabHelpers(web_contents, base::nullopt); > On 2017/05/23 20:29:57, Bryan McQuade wrote: > > On 2017/05/23 at 15:38:59, Avi (ping after 24h) wrote: > > > Is this not susceptible to the same issue? You may need to plumb this further > > down this call chain... > > > > This path doesn't appear to be the one that's exercised for the specific bug, > > but I agree it'd be better to provide the CreateParams here if available. > > > > Tracing the call chain, this is called from TabAndroid::InitWebContents, which > > gets the webcontents via WebContents::FromJavaWebContents (JNI). I wasn't able > > to figure out where if at all CreateParams are specified for constructing these > > WebContents, and it doesn't appear that they'd be available here given we've > > crossed the Java/Native boundary. > > > > So, I think we're unfortunately unable to get access to the CreateParams here, > > as best I can tell. Happy to add support here if there's a way to do it, though. > > It comes from https://cs.chromium.org/chromium/src/chrome/browser/android/web_contents_fact... > > I think probably should figure out a way to plumb the CreateParams here if it's needed. UserData maybe? Thanks! Nasko had encouraged us not to hold on to the CreateParams, due to the increased memory cost. Fortunately there isn't currently a need to have the CreateParams for this particular code flow at this time, so I propose that we leave it as unset here, and if in the future a consumer needs the params from this flow, we can revisit. I think the idea to UserData makes sense if we do decide to add support here. Thanks!
On 2017/05/24 00:00:50, Bryan McQuade wrote: > On 2017/05/23 at 21:16:28, boliu wrote: > > android_webview and components/web_contents_delegate_android rs lgtm > > > > I don't own chrome/android code, but probably needs some work. > > > > > https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/android... > > File chrome/browser/android/tab_android.cc (right): > > > > > https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/android... > > chrome/browser/android/tab_android.cc:122: > TabHelpers::AttachTabHelpers(web_contents, base::nullopt); > > On 2017/05/23 20:29:57, Bryan McQuade wrote: > > > On 2017/05/23 at 15:38:59, Avi (ping after 24h) wrote: > > > > Is this not susceptible to the same issue? You may need to plumb this > further > > > down this call chain... > > > > > > This path doesn't appear to be the one that's exercised for the specific > bug, > > > but I agree it'd be better to provide the CreateParams here if available. > > > > > > Tracing the call chain, this is called from TabAndroid::InitWebContents, > which > > > gets the webcontents via WebContents::FromJavaWebContents (JNI). I wasn't > able > > > to figure out where if at all CreateParams are specified for constructing > these > > > WebContents, and it doesn't appear that they'd be available here given we've > > > crossed the Java/Native boundary. > > > > > > So, I think we're unfortunately unable to get access to the CreateParams > here, > > > as best I can tell. Happy to add support here if there's a way to do it, > though. > > > > It comes from > https://cs.chromium.org/chromium/src/chrome/browser/android/web_contents_fact... > > > > I think probably should figure out a way to plumb the CreateParams here if > it's needed. UserData maybe? > > Thanks! Nasko had encouraged us not to hold on to the CreateParams, due to the > increased memory cost. Fortunately there isn't currently a need to have the > CreateParams for this particular code flow at this time, so I propose that we > leave it as unset here, and if in the future a consumer needs the params from > this flow, we can revisit. I think the idea to UserData makes sense if we do > decide to add support here. Thanks! I don't know what bug this CL intends to fix. But just looking at the code, it leaves things very brittle. Someone could change CreateParams in CreateWebContents in the future, and not know these other places needs to be updated as well. Will that be a problem?
lgtm For prerender. Which is "mostly deprecated" now, we don't like the edge cases more than anyone else...
headless/ lgtm.
devtools lgtm
On 2017/05/24 at 00:18:05, boliu wrote: > On 2017/05/24 00:00:50, Bryan McQuade wrote: > > On 2017/05/23 at 21:16:28, boliu wrote: > > > android_webview and components/web_contents_delegate_android rs lgtm > > > > > > I don't own chrome/android code, but probably needs some work. > > > > > > > > https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/android... > > > File chrome/browser/android/tab_android.cc (right): > > > > > > > > https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/android... > > > chrome/browser/android/tab_android.cc:122: > > TabHelpers::AttachTabHelpers(web_contents, base::nullopt); > > > On 2017/05/23 20:29:57, Bryan McQuade wrote: > > > > On 2017/05/23 at 15:38:59, Avi (ping after 24h) wrote: > > > > > Is this not susceptible to the same issue? You may need to plumb this > > further > > > > down this call chain... > > > > > > > > This path doesn't appear to be the one that's exercised for the specific > > bug, > > > > but I agree it'd be better to provide the CreateParams here if available. > > > > > > > > Tracing the call chain, this is called from TabAndroid::InitWebContents, > > which > > > > gets the webcontents via WebContents::FromJavaWebContents (JNI). I wasn't > > able > > > > to figure out where if at all CreateParams are specified for constructing > > these > > > > WebContents, and it doesn't appear that they'd be available here given we've > > > > crossed the Java/Native boundary. > > > > > > > > So, I think we're unfortunately unable to get access to the CreateParams > > here, > > > > as best I can tell. Happy to add support here if there's a way to do it, > > though. > > > > > > It comes from > > https://cs.chromium.org/chromium/src/chrome/browser/android/web_contents_fact... > > > > > > I think probably should figure out a way to plumb the CreateParams here if > > it's needed. UserData maybe? > > > > Thanks! Nasko had encouraged us not to hold on to the CreateParams, due to the > > increased memory cost. Fortunately there isn't currently a need to have the > > CreateParams for this particular code flow at this time, so I propose that we > > leave it as unset here, and if in the future a consumer needs the params from > > this flow, we can revisit. I think the idea to UserData makes sense if we do > > decide to add support here. Thanks! > > I don't know what bug this CL intends to fix. But just looking at the code, it leaves things very brittle. Someone could change CreateParams in CreateWebContents in the future, and not know these other places needs to be updated as well. Will that be a problem? Sure - there are more details in the linked bug and the CL description, but the focus here is on the specific callpath in chrome::Navigate(). With PlzNavigate enabled, in this specific codepath, for navigations in a new foreground tab, the NavigationHandle is created before the WebContents has been added to the tab strip. This causes page load metrics to infer that the navigation started in the background, since the WC reports that it is not yet foregrounded at the time the navigatoin starts, even though the issue is just an ordering issue: the WebContents is foregrounded, but the code to add it to the tabstrip runs right after the NavigationHandle is created. With this patch, page load metrics is able to ask the CreateParams whether the WC will be initially hidden or not, which allows us to accurately gather the initial foreground / background state of the WC, without being subject to the order of execution issues we face currently. While it would be nice to provide CreateParams for all possible code paths, it's only in chrome::Navigate that there's an execution ordering inversion, so there is not a current need for the CreateParams in any of the other callpaths at this time. I went ahead and added CreateParams to other codepaths where there was no cost to do so (such as holding on to a CreateParams for a longer period of time, increasing memory footprint) as part of this change. Should there be a need to have CreateParams for some of these other codepaths in the future, we can revisit at that time, but until that's the case, we should avoid keeping the CreateParams around (using the UserData approach you suggested for instance) for any longer than needed.
https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/android... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/android... chrome/browser/android/tab_android.cc:122: TabHelpers::AttachTabHelpers(web_contents, base::nullopt); On 2017/05/23 at 21:16:28, boliu wrote: > On 2017/05/23 20:29:57, Bryan McQuade wrote: > > On 2017/05/23 at 15:38:59, Avi (ping after 24h) wrote: > > > Is this not susceptible to the same issue? You may need to plumb this further > > down this call chain... > > > > This path doesn't appear to be the one that's exercised for the specific bug, > > but I agree it'd be better to provide the CreateParams here if available. > > > > Tracing the call chain, this is called from TabAndroid::InitWebContents, which > > gets the webcontents via WebContents::FromJavaWebContents (JNI). I wasn't able > > to figure out where if at all CreateParams are specified for constructing these > > WebContents, and it doesn't appear that they'd be available here given we've > > crossed the Java/Native boundary. > > > > So, I think we're unfortunately unable to get access to the CreateParams here, > > as best I can tell. Happy to add support here if there's a way to do it, though. > > It comes from https://cs.chromium.org/chromium/src/chrome/browser/android/web_contents_fact... > > I think probably should figure out a way to plumb the CreateParams here if it's needed. UserData maybe? Copying the earlier discussion into the appropriate comment thread, to make it clearer that this has been addressed: The focus of this CL is addressing an issue caused by the specific callpath in chrome::Navigate(). With PlzNavigate enabled, in this specific codepath, for navigations in a new foreground tab, the NavigationHandle is created before the WebContents has been added to the tab strip. This causes page load metrics to infer that the navigation started in the background, since the WC reports that it is not yet foregrounded at the time the navigatoin starts, even though the issue is just an ordering issue: the WebContents is foregrounded, but the code to add it to the tabstrip runs right after the NavigationHandle is created. With this patch, page load metrics is able to ask the CreateParams whether the WC will be initially hidden or not, which allows us to accurately gather the initial foreground / background state of the WC, without being subject to the order of execution issues we face currently. While it would be nice to provide CreateParams for all possible code paths, it's only in chrome::Navigate that there's an execution ordering inversion, so there is not a current need for the CreateParams in any of the other callpaths at this time. I went ahead and added CreateParams to other codepaths where there was no cost to do so (such as holding on to a CreateParams for a longer period of time, increasing memory footprint) as part of this change. Should there be a need to have CreateParams for some of these other codepaths in the future, we can revisit at that time, but until that's the case, we should avoid keeping the CreateParams around (using the UserData approach you suggested for instance) for any longer than needed.
guest_view lgtm
lgtm assuming the answer to my comment is yes https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/android... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/android... chrome/browser/android/tab_android.cc:122: TabHelpers::AttachTabHelpers(web_contents, base::nullopt); So do the metrics work correctly on Android? Is that something that's been checked? On 2017/05/25 03:19:12, Bryan McQuade wrote: > On 2017/05/23 at 21:16:28, boliu wrote: > > On 2017/05/23 20:29:57, Bryan McQuade wrote: > > > On 2017/05/23 at 15:38:59, Avi (ping after 24h) wrote: > > > > Is this not susceptible to the same issue? You may need to plumb this > further > > > down this call chain... > > > > > > This path doesn't appear to be the one that's exercised for the specific > bug, > > > but I agree it'd be better to provide the CreateParams here if available. > > > > > > Tracing the call chain, this is called from TabAndroid::InitWebContents, > which > > > gets the webcontents via WebContents::FromJavaWebContents (JNI). I wasn't > able > > > to figure out where if at all CreateParams are specified for constructing > these > > > WebContents, and it doesn't appear that they'd be available here given we've > > > crossed the Java/Native boundary. > > > > > > So, I think we're unfortunately unable to get access to the CreateParams > here, > > > as best I can tell. Happy to add support here if there's a way to do it, > though. > > > > It comes from > https://cs.chromium.org/chromium/src/chrome/browser/android/web_contents_fact... > > > > I think probably should figure out a way to plumb the CreateParams here if > it's needed. UserData maybe? > > Copying the earlier discussion into the appropriate comment thread, to make it > clearer that this has been addressed: > > The focus of this CL is addressing an issue caused by the specific callpath in > chrome::Navigate(). With PlzNavigate enabled, in this specific codepath, for > navigations in a new foreground tab, the NavigationHandle is created before the > WebContents has been added to the tab strip. This causes page load metrics to > infer that the navigation started in the background, since the WC reports that > it is not yet foregrounded at the time the navigatoin starts, even though the > issue is just an ordering issue: the WebContents is foregrounded, but the code > to add it to the tabstrip runs right after the NavigationHandle is created. With > this patch, page load metrics is able to ask the CreateParams whether the WC > will be initially hidden or not, which allows us to accurately gather the > initial foreground / background state of the WC, without being subject to the > order of execution issues we face currently. > > While it would be nice to provide CreateParams for all possible code paths, it's > only in chrome::Navigate that there's an execution ordering inversion, so there > is not a current need for the CreateParams in any of the other callpaths at this > time. I went ahead and added CreateParams to other codepaths where there was no > cost to do so (such as holding on to a CreateParams for a longer period of time, > increasing memory footprint) as part of this change. > > Should there be a need to have CreateParams for some of these other codepaths in > the future, we can revisit at that time, but until that's the case, we should > avoid keeping the CreateParams around (using the UserData approach you suggested > for instance) for any longer than needed.
Note: I'm behind on codereviews but will try and get to this today (Thursday May 25), after I get some sleep.
LGTM https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/ui/tab_... File chrome/browser/ui/tab_helpers.h (right): https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/ui/tab_... chrome/browser/ui/tab_helpers.h:9: #include "content/public/browser/web_contents.h" Do you really need to #include web_contents.h? I'd think the forward-decl of it before would continue to work.
The CQ bit was checked by bmcquade@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...
On 2017/05/25 at 23:21:44, pkasting wrote: > LGTM > > https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/ui/tab_... > File chrome/browser/ui/tab_helpers.h (right): > > https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/ui/tab_... > chrome/browser/ui/tab_helpers.h:9: #include "content/public/browser/web_contents.h" > Do you really need to #include web_contents.h? I'd think the forward-decl of it before would continue to work. My understanding is that because WebContents::CreateParams is a nested class, it's not possible to forward declare it. Let me know if I'm mistaken here though - happy to forward declare if that's possible. Thanks!
https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/android... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/android... chrome/browser/android/tab_android.cc:122: TabHelpers::AttachTabHelpers(web_contents, base::nullopt); On 2017/05/25 at 05:12:20, Maria wrote: > So do the metrics work correctly on Android? Is that something that's been checked? > > On 2017/05/25 03:19:12, Bryan McQuade wrote: > > On 2017/05/23 at 21:16:28, boliu wrote: > > > On 2017/05/23 20:29:57, Bryan McQuade wrote: > > > > On 2017/05/23 at 15:38:59, Avi (ping after 24h) wrote: > > > > > Is this not susceptible to the same issue? You may need to plumb this > > further > > > > down this call chain... > > > > > > > > This path doesn't appear to be the one that's exercised for the specific > > bug, > > > > but I agree it'd be better to provide the CreateParams here if available. > > > > > > > > Tracing the call chain, this is called from TabAndroid::InitWebContents, > > which > > > > gets the webcontents via WebContents::FromJavaWebContents (JNI). I wasn't > > able > > > > to figure out where if at all CreateParams are specified for constructing > > these > > > > WebContents, and it doesn't appear that they'd be available here given we've > > > > crossed the Java/Native boundary. > > > > > > > > So, I think we're unfortunately unable to get access to the CreateParams > > here, > > > > as best I can tell. Happy to add support here if there's a way to do it, > > though. > > > > > > It comes from > > https://cs.chromium.org/chromium/src/chrome/browser/android/web_contents_fact... > > > > > > I think probably should figure out a way to plumb the CreateParams here if > > it's needed. UserData maybe? > > > > Copying the earlier discussion into the appropriate comment thread, to make it > > clearer that this has been addressed: > > > > The focus of this CL is addressing an issue caused by the specific callpath in > > chrome::Navigate(). With PlzNavigate enabled, in this specific codepath, for > > navigations in a new foreground tab, the NavigationHandle is created before the > > WebContents has been added to the tab strip. This causes page load metrics to > > infer that the navigation started in the background, since the WC reports that > > it is not yet foregrounded at the time the navigatoin starts, even though the > > issue is just an ordering issue: the WebContents is foregrounded, but the code > > to add it to the tabstrip runs right after the NavigationHandle is created. With > > this patch, page load metrics is able to ask the CreateParams whether the WC > > will be initially hidden or not, which allows us to accurately gather the > > initial foreground / background state of the WC, without being subject to the > > order of execution issues we face currently. > > > > While it would be nice to provide CreateParams for all possible code paths, it's > > only in chrome::Navigate that there's an execution ordering inversion, so there > > is not a current need for the CreateParams in any of the other callpaths at this > > time. I went ahead and added CreateParams to other codepaths where there was no > > cost to do so (such as holding on to a CreateParams for a longer period of time, > > increasing memory footprint) as part of this change. > > > > Should there be a need to have CreateParams for some of these other codepaths in > > the future, we can revisit at that time, but until that's the case, we should > > avoid keeping the CreateParams around (using the UserData approach you suggested > > for instance) for any longer than needed. Metrics work on Android as well. This change includes a unit test for the case fixed by the change, and that test passes on android. https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/ui/tab_... File chrome/browser/ui/tab_helpers.h (right): https://codereview.chromium.org/2894973002/diff/300001/chrome/browser/ui/tab_... chrome/browser/ui/tab_helpers.h:9: #include "content/public/browser/web_contents.h" On 2017/05/25 at 23:21:44, Peter Kasting wrote: > Do you really need to #include web_contents.h? I'd think the forward-decl of it before would continue to work. WebContents::CreateParams is an inner class of WebContents. My understanding is there's no way to forward declare a nested class like this. Let me know if I'm mistaken - very happy to forward declare if it is possible to do so.
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 bmcquade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, pkasting@chromium.org, mariakhomenko@chromium.org, mattcary@chromium.org, boliu@chromium.org, fsamuel@chromium.org, dgozman@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2894973002/#ps320001 (title: "rebase")
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": 320001, "attempt_start_ts": 1495768075318500,
"parent_rev": "a669662d38b1c4a8071ea4cde7a904b2703e0e71", "commit_rev":
"42b72063edb66bfd015bd9dea116c3e3e937d3eb"}
Message was sent while issue was closed.
Description was changed from ========== Provide WebContents::CreateParams to tab helpers. With browser-side navigation enabled, navigations in new foreground tabs are considered to have started in the background. This is a change in behavior. This causes page load metrics to incorrectly consider these page loads to have started in the background, which skews all of Chrome's page load metrics (PageLoad.* in UMA). clamy explains: """ the background/foreground issue seems to be due to the ordering of calls in chrome/browser/ui/browser_navigator.cc when we create a new tab. What happens is: - we first create a WebContents and ask it to navigate (https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_navigator.cc?t...). This result in the creation of a NavigationHandle, so we fire DidStartNavigation. In the current architecture, we fire DidStartNavigation after receiving DidStartProvisionalLoad. - then we insert the WebContents in the tab strip (https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_navigator.cc?t...). This fires WebContentsImpl::WasShown. """ This patch implements Nasko's proposed fix to expose the WebContents CreateParams to interested WebContentsObservers, page load metrics's MetricsWebContentsObserver being the first consumer. We don't have the CreateParams in all places where a WebContents is passed to TabHelpers, so we wrap the CreateParams in a base::Optional<> and provide CreateParams in cases where they are available. BUG=725347 ========== to ========== Provide WebContents::CreateParams to tab helpers. With browser-side navigation enabled, navigations in new foreground tabs are considered to have started in the background. This is a change in behavior. This causes page load metrics to incorrectly consider these page loads to have started in the background, which skews all of Chrome's page load metrics (PageLoad.* in UMA). clamy explains: """ the background/foreground issue seems to be due to the ordering of calls in chrome/browser/ui/browser_navigator.cc when we create a new tab. What happens is: - we first create a WebContents and ask it to navigate (https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_navigator.cc?t...). This result in the creation of a NavigationHandle, so we fire DidStartNavigation. In the current architecture, we fire DidStartNavigation after receiving DidStartProvisionalLoad. - then we insert the WebContents in the tab strip (https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_navigator.cc?t...). This fires WebContentsImpl::WasShown. """ This patch implements Nasko's proposed fix to expose the WebContents CreateParams to interested WebContentsObservers, page load metrics's MetricsWebContentsObserver being the first consumer. We don't have the CreateParams in all places where a WebContents is passed to TabHelpers, so we wrap the CreateParams in a base::Optional<> and provide CreateParams in cases where they are available. BUG=725347 Review-Url: https://codereview.chromium.org/2894973002 Cr-Commit-Position: refs/heads/master@{#474895} Committed: https://chromium.googlesource.com/chromium/src/+/42b72063edb66bfd015bd9dea116... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/chromium/src/+/42b72063edb66bfd015bd9dea116...
Message was sent while issue was closed.
A revert of this CL (patchset #17 id:320001) has been created in https://codereview.chromium.org/2983533002/ by bmcquade@chromium.org. The reason for reverting is: sky@ and ducboi@ found a simpler and more correct fix in https://chromium-review.googlesource.com/c/569578/, so this is no longer needed.. |
