New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 725347 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

browser-side navigations in new tabs are recorded as starting in the background

Project Member Reported by bmcquade@chromium.org, May 23 2017

Issue description

With browser-side navigation enabled, navigations in new foreground tabs are considered to have started in the background.

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 a 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 is a change in behavior from before PlzNavigate. 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).

Nasko proposes fixing this by exposing the WebContents CreationParams to interested WebContentsObservers:
"""
We create the WebContents and immediately after attach all TabHelpers. We have the creation parameters, which indicate the disposition of the new tab. What if we were to pass these parameters to the construction of TabHelpers. Then your TabHelper can check the disposition and calculate the initial visibility state - current tab and new foreground tab will be visible, while new background tab will be invisible.
"""

We should implement Nasko's suggestion and pass the CreateParams on to the MetricsWebContentsObserver, which keeps track of fg/bg state for page load metrics.

Nasko notes that we won't have the CreateParams in all places where a WebContents is passed to TabHelpers, so we should wrap in base::Optional<> and provide the CreateParams where they are available.
 
Cc: nduca@chromium.org clamy@chromium.org zh...@chromium.org bmcquade@chromium.org nasko@chromium.org
 Issue 716544  has been merged into this issue.
Project Member

Comment 2 by bugdroid1@chromium.org, May 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/42b72063edb66bfd015bd9dea116c3e3e937d3eb

commit 42b72063edb66bfd015bd9dea116c3e3e937d3eb
Author: bmcquade <bmcquade@chromium.org>
Date: Fri May 26 03:14:11 2017

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?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}

[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/android_webview/browser/aw_web_contents_delegate.cc
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/android_webview/browser/aw_web_contents_delegate.h
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/chrome/browser/android/tab_android.cc
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/chrome/browser/android/tab_web_contents_delegate_android.cc
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/chrome/browser/devtools/devtools_window.cc
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/chrome/browser/devtools/devtools_window.h
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/chrome/browser/page_load_metrics/metrics_web_contents_observer.h
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/chrome/browser/page_load_metrics/page_load_metrics_initialize.cc
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/chrome/browser/page_load_metrics/page_load_metrics_initialize.h
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/chrome/browser/prerender/prerender_contents.cc
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/chrome/browser/prerender/prerender_contents.h
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/chrome/browser/ui/browser.cc
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/chrome/browser/ui/browser.h
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/chrome/browser/ui/browser_navigator.cc
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/chrome/browser/ui/browser_tab_strip_model_delegate.cc
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/chrome/browser/ui/tab_helpers.cc
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/chrome/browser/ui/tab_helpers.h
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/components/web_contents_delegate_android/web_contents_delegate_android.cc
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/components/web_contents_delegate_android/web_contents_delegate_android.h
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/content/public/browser/web_contents_delegate.h
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/extensions/browser/guest_view/web_view/web_view_guest.cc
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/extensions/browser/guest_view/web_view/web_view_guest.h
[modify] https://crrev.com/42b72063edb66bfd015bd9dea116c3e3e937d3eb/headless/lib/browser/headless_web_contents_impl.cc

Status: Fixed (was: Started)
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d2b93c04c6f18c3c405668757a2e463ae15758e3

commit d2b93c04c6f18c3c405668757a2e463ae15758e3
Author: Bryan McQuade <bmcquade@chromium.org>
Date: Fri Jul 14 22:26:31 2017

Partial revert of Provide WebContents::CreateParams to tab helpers.

Reason for revert:
sky@ and ducbui@ found a simpler and more correct fix in
https://chromium-review.googlesource.com/c/569578/, so this is no longer needed.

The browsertest NewPageInNewForegroundTab, as well as prerender logic in the
MetricsWebContentsObserver constructor, are not reverted, as the browsertest
is expected to continue to pass, and the prerender logic is still needed
even with ducbui's fix.

Original issue's description:
> 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?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


Bug:  725347 
Change-Id: I10871fb8a711e9a71b1b8ad2541b6bce017d27be
Reviewed-on: https://chromium-review.googlesource.com/571285
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Bryan McQuade <bmcquade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486909}
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/android_webview/browser/aw_web_contents_delegate.cc
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/android_webview/browser/aw_web_contents_delegate.h
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/chrome/browser/android/tab_android.cc
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/chrome/browser/android/tab_web_contents_delegate_android.cc
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/chrome/browser/devtools/devtools_window.cc
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/chrome/browser/devtools/devtools_window.h
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/chrome/browser/page_load_metrics/metrics_web_contents_observer.h
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/chrome/browser/page_load_metrics/page_load_metrics_initialize.cc
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/chrome/browser/page_load_metrics/page_load_metrics_initialize.h
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/chrome/browser/prerender/prerender_contents.cc
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/chrome/browser/prerender/prerender_contents.h
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/chrome/browser/ui/browser.cc
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/chrome/browser/ui/browser.h
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/chrome/browser/ui/browser_navigator.cc
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/chrome/browser/ui/browser_tab_strip_model_delegate.cc
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/chrome/browser/ui/tab_helpers.cc
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/chrome/browser/ui/tab_helpers.h
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/components/web_contents_delegate_android/web_contents_delegate_android.cc
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/components/web_contents_delegate_android/web_contents_delegate_android.h
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/content/public/browser/web_contents_delegate.h
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/extensions/browser/guest_view/web_view/web_view_guest.cc
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/extensions/browser/guest_view/web_view/web_view_guest.h
[modify] https://crrev.com/d2b93c04c6f18c3c405668757a2e463ae15758e3/headless/lib/browser/headless_web_contents_impl.cc

Sign in to add a comment