Provide WebContents::CreateParams to tab helpers.

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

[Bryan McQuade, 2017-05-19 20:00:02]

The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run

[Bryan McQuade, 2017-05-19 20:00:30]

Description was changed from

==========
Partial impl to provide WebContents::CreateParams to tab helpers.

BUG=
==========

to

==========
Provide WebContents::CreateParams to tab helpers.

BUG=
==========

[commit-bot: I haz the power, 2017-05-19 20:01:01]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-05-19 20:01:31]

The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 20:02:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 20:33:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-19 20:33:56]

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_...)

[Bryan McQuade, 2017-05-22 12:36:51]

The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 12:37:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 13:07:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 13:07:28]

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_...)

[Bryan McQuade, 2017-05-22 14:05:14]

The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 14:05:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 14:33:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 14:33:08]

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...)

[Bryan McQuade, 2017-05-22 14:53:32]

The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 14:53:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-05-22 14:56:08]

The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 14:56:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 15:26:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 15:26:47]

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_...)

[Bryan McQuade, 2017-05-22 16:49:30]

The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 16:49:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 17:17:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 17:17:33]

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...)

[Bryan McQuade, 2017-05-22 18:10:27]

The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 18:11:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 18:36:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 18:36:43]

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_...)

[Bryan McQuade, 2017-05-22 19:24:17]

The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 19:24:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-05-22 19:27:16]

The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 19:27:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-05-22 20:02:38]

The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 20:03:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 21:36:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 21:36:15]

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_...)

[Bryan McQuade, 2017-05-22 23:09:19]

The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 23:09:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 23:56:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 23:56:17]

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_...)

[Bryan McQuade, 2017-05-23 00:07:17]

The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 00:08:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 01:43:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-23 01:43:52]

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_...)

[Bryan McQuade, 2017-05-23 04:08:14]

Description was changed from

==========
Provide WebContents::CreateParams to tab helpers.

BUG=
==========

to

==========
Provide WebContents::CreateParams to tab helpers.

BUG=725347
==========

[Bryan McQuade, 2017-05-23 04:08:28]

The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 04:08:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 04:10:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-23 04:10:58]

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...)

[Bryan McQuade, 2017-05-23 04:11:33]

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
==========

[Bryan McQuade, 2017-05-23 04:11:46]

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
==========

[Bryan McQuade, 2017-05-23 04:17:05]

The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 04:17:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 05:20:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-23 05:21:01]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2017-05-23 15:38:57]

avi@chromium.org changed reviewers:
+ avi@chromium.org

[Avi (use Gerrit), 2017-05-23 15:38:59]

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 🙄

[Bryan McQuade, 2017-05-23 20:29:57]

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.

[Bryan McQuade, 2017-05-23 20:35:47]

bmcquade@chromium.org changed reviewers:
+ boliu@chromium.org, dgozman@chromium.org, fsamuel@chromium.org,
mariakhomenko@chromium.org, mattcary@chromium.org, pkasting@chromium.org,
skyostil@chromium.org

[Bryan McQuade, 2017-05-23 20:37:35]

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!

[Avi (use Gerrit), 2017-05-23 20:49:54]

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.

[boliu, 2017-05-23 21:16:28]

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?

[Bryan McQuade, 2017-05-24 00:00:50]

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!

[boliu, 2017-05-24 00:18:05]

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?

[mattcary, 2017-05-24 08:01:54]

lgtm

For prerender. Which is "mostly deprecated" now, we don't like the edge cases
more than anyone else...

[Sami, 2017-05-24 17:43:37]

headless/ lgtm.

[dgozman, 2017-05-24 19:16:53]

devtools lgtm

[Bryan McQuade, 2017-05-25 03:08:24]

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.

[Bryan McQuade, 2017-05-25 03:19:12]

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.

[Fady Samuel, 2017-05-25 04:07:01]

guest_view lgtm

[Maria, 2017-05-25 05:12:20]

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.

[Peter Kasting, 2017-05-25 07:24:58]

Note: I'm behind on codereviews but will try and get to this today (Thursday May
25), after I get some sleep.

[Peter Kasting, 2017-05-25 23:21:44]

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.

[Bryan McQuade, 2017-05-26 00:25:40]

The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 00:27:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-05-26 00:38:47]

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!

[Bryan McQuade, 2017-05-26 00:39:44]

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.

[commit-bot: I haz the power, 2017-05-26 03:06:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-26 03:06:10]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-05-26 03:07:55]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2017-05-26 03:07:55]

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")

[commit-bot: I haz the power, 2017-05-26 03:08:46]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 03:14:16]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1495768075318500,
"parent_rev": "a669662d38b1c4a8071ea4cde7a904b2703e0e71", "commit_rev":
"42b72063edb66bfd015bd9dea116c3e3e937d3eb"}

[commit-bot: I haz the power, 2017-05-26 03:14:28]

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...
==========

[commit-bot: I haz the power, 2017-05-26 03:14:30]

Committed patchset #17 (id:320001) as
https://chromium.googlesource.com/chromium/src/+/42b72063edb66bfd015bd9dea116...

[Bryan McQuade, 2017-07-14 00:02:23]

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..