Chromium Code Reviews Chromium Code Reviews
Issue
2983533002:
Revert of Provide WebContents::CreateParams to tab helpers. (Closed)
DescriptionRevert of Provide WebContents::CreateParams to tab helpers. (patchset #17 id:320001 of https://codereview.chromium.org/2894973002/ )
Reason for revert:
sky@ and ducboi@ found a simpler and more correct fix in https://chromium-review.googlesource.com/c/569578/, so this is no longer needed.
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
TBR=avi@chromium.org,boliu@chromium.org,dgozman@chromium.org,fsamuel@chromium.org,mariakhomenko@chromium.org,mattcary@chromium.org,pkasting@chromium.org,skyostil@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=725347
Patch Set 1 #Messages
Total messages: 4 (2 generated)
|
