Propagate opener to BackgroundsContents.

Propagate opener to BackgroundsContents.

Before this CL, the opener would be lost:
1. When creating the BackgroundContents:
    1.1. The call from WebContentsImpl::CreateNewWindow to
         Browser::ShouldCreateWebContents didn't propagate |opener|
    1.2. WebContents::CreateWithSessionStorage ignored opener-related
         fields in WebContents::CreateParams.
2. When inspecting the opener - WebContents::GetOpener used to return
   WebContents rather than RenderFrameHost (misrepresenting an opener
   that is a subframe).

This CL fixes this by:
1. Adding an |opener| parameter to WebContentsDelegate::ShouldCreateWebContents
2. Modifying constructor of BackgroundContents to take a new |opener|
   parameter and to pass it to WebContents::CreateParams.
3. Modifying WebContents::CreateWithSessionStorage so that it sets
   the opener on the newly created contents.
4. Changing WebContents::GetOpener and WebContents::GetOriginalOpener
   methods to return RenderFrameHost.

The accurate opener was originally needed for a planned but ultimately
abandoned UMA (see https://crbug.com/718516), but having an accurate
opener is desirable in general.

This CL re-enables AppBackgroundPageApiTest.OpenThenClose which was
disabled due to flakiness 4 years ago.  This test is where the
verification of the opener has to go.  I cannot repro the flakiness when
trying the test locally (i.e. running the test 30 times individually,
and 20 times as part of the whole AppBackgroundPageApiTest suite) or on
the trybots (I've tried 3 x win_chromium_rel_ng, linux_chromium_rel_ng
and mac_chromium_rel_ng).

BUG=165644, 724280
Review-Url: https://codereview.chromium.org/2882513005
Cr-Commit-Position: refs/heads/master@{#477169}
Committed: https://chromium.googlesource.com/chromium/src/+/6f8ac625db8fc02373327a9957e6633b62717d0a

[Łukasz Anforowicz, 2017-05-11 23:10:47]

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

[commit-bot: I haz the power, 2017-05-11 23:12:15]

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-11 23:34:10]

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

[commit-bot: I haz the power, 2017-05-11 23:34:11]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Łukasz Anforowicz, 2017-05-11 23:59:44]

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

[commit-bot: I haz the power, 2017-05-12 00:00:34]

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-12 00:29:22]

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

[commit-bot: I haz the power, 2017-05-12 00:29:23]

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

[Łukasz Anforowicz, 2017-05-12 00:54:08]

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

[commit-bot: I haz the power, 2017-05-12 00:55:10]

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-12 01:26:18]

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

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

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

[Łukasz Anforowicz, 2017-05-12 03:42:36]

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

[commit-bot: I haz the power, 2017-05-12 03:43:13]

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-12 05:08:29]

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

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

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Łukasz Anforowicz, 2017-05-12 18:20:59]

lukasza@chromium.org changed reviewers:
+ nick@chromium.org

[Łukasz Anforowicz, 2017-05-12 18:20:59]

nick@, could you PTAL?

Without this CL, BackgroundContents won't learn about its opener until the *2nd*
call to window.open.

[ncarter (slow), 2017-05-17 20:24:28]

Description was changed from

==========
Propagate opener to BackgroundsCountents.

Before this CL, the opener would be lost, because:
1. The call from WebContentsImpl::CreateNewWindow to
   Browser::ShouldCreateWebContents doesn't propagate |opener|
2. WebContents::CreateWithSessionStorage ignores opener-related
   fields in WebContents::CreateParams.

Missing an initial opener in BackgroundContents made it difficult
to realize if the opener changes (i.e. when BackgroundContents
are used simultaneously form 2 separate tabs).  This meant that
gathering reliable UMA data was impossible.

This CL fixes this by:
1. Adding an |opener| parameter to WebContentsDelegate::ShouldCreateWebContents
2. Modifying constructor of BackgroundContents to take a new |opener|
   parameter and to pass it to WebContents::CreateParams.
3. Modifying WebContents::CreateWithSessionStorage so that it sets
   the opener on the newly created contents.

BUG=718516
==========

to

==========
Propagate opener to BackgroundsContents.

Before this CL, the opener would be lost, because:
1. The call from WebContentsImpl::CreateNewWindow to
   Browser::ShouldCreateWebContents doesn't propagate |opener|
2. WebContents::CreateWithSessionStorage ignores opener-related
   fields in WebContents::CreateParams.

Missing an initial opener in BackgroundContents made it difficult
to realize if the opener changes (i.e. when BackgroundContents
are used simultaneously form 2 separate tabs).  This meant that
gathering reliable UMA data was impossible.

This CL fixes this by:
1. Adding an |opener| parameter to WebContentsDelegate::ShouldCreateWebContents
2. Modifying constructor of BackgroundContents to take a new |opener|
   parameter and to pass it to WebContents::CreateParams.
3. Modifying WebContents::CreateWithSessionStorage so that it sets
   the opener on the newly created contents.

BUG=718516
==========

[ncarter (slow), 2017-05-17 20:45:27]

https://codereview.chromium.org/2882513005/diff/60001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/2882513005/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:2625: opener, MSG_ROUTING_NONE, MSG_ROUTING_NONE,
MSG_ROUTING_NONE, profile_,
this should be nullptr.

preserving the |opener| here is, I think incorrect: the opener doesn't get a
reference to |contents| -- and I'm pretty sure window.opener in the background
contents' document will be null.

https://codereview.chromium.org/2882513005/diff/60001/components/offline_page...
File
components/offline_pages/content/background_loader/background_loader_contents_unittest.cc
(right):

https://codereview.chromium.org/2882513005/diff/60001/components/offline_page...
components/offline_pages/content/background_loader/background_loader_contents_unittest.cc:97:
TEST_F(BackgroundLoaderContentsTest, ShouldNotCreateWebContents) {
Could you also add a few lines to app_background_page_apitest.cc to test the
changes? The idea is to look for consistency between the nullness of
|window.opener| in the background page, and the value of
|WebContents::GetOpener()|.

Near the end of TEST_F(AppBackgroundPageApiTest, NoJsBackgroundPage):

 BackgroundContents* background_contents =
BackgroundContentsServiceFactory::GetForProfile(browser()->profile())->GetAppBackgroundContents(ASCIIToUTF16(extension->id());

 ASSERT_TRUE(background_contents);
 ASSERT_FALSE(background_contents->web_contents()->GetOpener());

[and maybe, also, ExecuteScriptAndExtractBool(web_contents()->GetMainFrame(),
"send(window.opener == null)"]

And do similar but opposite checks in a test w/ js access (is there even one
there?)

[Łukasz Anforowicz, 2017-05-18 21:22:10]

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

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

Dry run: CQ is trying da patch.

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

[Łukasz Anforowicz, 2017-05-18 21:37:04]

Description was changed from

==========
Propagate opener to BackgroundsContents.

Before this CL, the opener would be lost, because:
1. The call from WebContentsImpl::CreateNewWindow to
   Browser::ShouldCreateWebContents doesn't propagate |opener|
2. WebContents::CreateWithSessionStorage ignores opener-related
   fields in WebContents::CreateParams.

Missing an initial opener in BackgroundContents made it difficult
to realize if the opener changes (i.e. when BackgroundContents
are used simultaneously form 2 separate tabs).  This meant that
gathering reliable UMA data was impossible.

This CL fixes this by:
1. Adding an |opener| parameter to WebContentsDelegate::ShouldCreateWebContents
2. Modifying constructor of BackgroundContents to take a new |opener|
   parameter and to pass it to WebContents::CreateParams.
3. Modifying WebContents::CreateWithSessionStorage so that it sets
   the opener on the newly created contents.

BUG=718516
==========

to

==========
Propagate opener to BackgroundsContents.

Before this CL, the opener would be lost:
1. When creating the BackgroundContents:
    1.1. The call from WebContentsImpl::CreateNewWindow to
         Browser::ShouldCreateWebContents didn't propagate |opener|
    1.2. WebContents::CreateWithSessionStorage ignored opener-related
         fields in WebContents::CreateParams.
2. When inspecting the opener - WebContents::GetOpener used to return
   WebContents rather than RenderFrameHost (misrepresenting an opener
   that is a subframe).

This CL fixes this by:
1. Adding an |opener| parameter to WebContentsDelegate::ShouldCreateWebContents
2. Modifying constructor of BackgroundContents to take a new |opener|
   parameter and to pass it to WebContents::CreateParams.
3. Modifying WebContents::CreateWithSessionStorage so that it sets
   the opener on the newly created contents.
4. Changing WebContents::GetOpener and WebContents::GetOriginalOpener
   methods to return RenderFrameHost.

The accurate opener was originally needed for planned UMA (see
https://crbug.com/718516), but having an accurate opener is desirable in
general.

This CL re-enables AppBackgroundPageApiTest.OpenThenClose which was
disabled due to flakiness 4 years ago.  This test is where the
verification of the opener has to go.  I cannot repro the flakiness when
trying the test locally.

BUG=165644, 718516
==========

[Łukasz Anforowicz, 2017-05-18 21:41:16]

Description was changed from

==========
Propagate opener to BackgroundsContents.

Before this CL, the opener would be lost:
1. When creating the BackgroundContents:
    1.1. The call from WebContentsImpl::CreateNewWindow to
         Browser::ShouldCreateWebContents didn't propagate |opener|
    1.2. WebContents::CreateWithSessionStorage ignored opener-related
         fields in WebContents::CreateParams.
2. When inspecting the opener - WebContents::GetOpener used to return
   WebContents rather than RenderFrameHost (misrepresenting an opener
   that is a subframe).

This CL fixes this by:
1. Adding an |opener| parameter to WebContentsDelegate::ShouldCreateWebContents
2. Modifying constructor of BackgroundContents to take a new |opener|
   parameter and to pass it to WebContents::CreateParams.
3. Modifying WebContents::CreateWithSessionStorage so that it sets
   the opener on the newly created contents.
4. Changing WebContents::GetOpener and WebContents::GetOriginalOpener
   methods to return RenderFrameHost.

The accurate opener was originally needed for planned UMA (see
https://crbug.com/718516), but having an accurate opener is desirable in
general.

This CL re-enables AppBackgroundPageApiTest.OpenThenClose which was
disabled due to flakiness 4 years ago.  This test is where the
verification of the opener has to go.  I cannot repro the flakiness when
trying the test locally.

BUG=165644, 718516
==========

to

==========
Propagate opener to BackgroundsContents.

Before this CL, the opener would be lost:
1. When creating the BackgroundContents:
    1.1. The call from WebContentsImpl::CreateNewWindow to
         Browser::ShouldCreateWebContents didn't propagate |opener|
    1.2. WebContents::CreateWithSessionStorage ignored opener-related
         fields in WebContents::CreateParams.
2. When inspecting the opener - WebContents::GetOpener used to return
   WebContents rather than RenderFrameHost (misrepresenting an opener
   that is a subframe).

This CL fixes this by:
1. Adding an |opener| parameter to WebContentsDelegate::ShouldCreateWebContents
2. Modifying constructor of BackgroundContents to take a new |opener|
   parameter and to pass it to WebContents::CreateParams.
3. Modifying WebContents::CreateWithSessionStorage so that it sets
   the opener on the newly created contents.
4. Changing WebContents::GetOpener and WebContents::GetOriginalOpener
   methods to return RenderFrameHost.

The accurate opener was originally needed for a planned but ultimately
abandoned UMA (see https://crbug.com/718516), but having an accurate
opener is desirable in general.

This CL re-enables AppBackgroundPageApiTest.OpenThenClose which was
disabled due to flakiness 4 years ago.  This test is where the
verification of the opener has to go.  I cannot repro the flakiness when
trying the test locally.

BUG=165644, 724280
==========

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

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

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

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2017-05-22 17:15:56]

Description was changed from

==========
Propagate opener to BackgroundsContents.

Before this CL, the opener would be lost:
1. When creating the BackgroundContents:
    1.1. The call from WebContentsImpl::CreateNewWindow to
         Browser::ShouldCreateWebContents didn't propagate |opener|
    1.2. WebContents::CreateWithSessionStorage ignored opener-related
         fields in WebContents::CreateParams.
2. When inspecting the opener - WebContents::GetOpener used to return
   WebContents rather than RenderFrameHost (misrepresenting an opener
   that is a subframe).

This CL fixes this by:
1. Adding an |opener| parameter to WebContentsDelegate::ShouldCreateWebContents
2. Modifying constructor of BackgroundContents to take a new |opener|
   parameter and to pass it to WebContents::CreateParams.
3. Modifying WebContents::CreateWithSessionStorage so that it sets
   the opener on the newly created contents.
4. Changing WebContents::GetOpener and WebContents::GetOriginalOpener
   methods to return RenderFrameHost.

The accurate opener was originally needed for a planned but ultimately
abandoned UMA (see https://crbug.com/718516), but having an accurate
opener is desirable in general.

This CL re-enables AppBackgroundPageApiTest.OpenThenClose which was
disabled due to flakiness 4 years ago.  This test is where the
verification of the opener has to go.  I cannot repro the flakiness when
trying the test locally.

BUG=165644, 724280
==========

to

==========
Propagate opener to BackgroundsContents.

Before this CL, the opener would be lost:
1. When creating the BackgroundContents:
    1.1. The call from WebContentsImpl::CreateNewWindow to
         Browser::ShouldCreateWebContents didn't propagate |opener|
    1.2. WebContents::CreateWithSessionStorage ignored opener-related
         fields in WebContents::CreateParams.
2. When inspecting the opener - WebContents::GetOpener used to return
   WebContents rather than RenderFrameHost (misrepresenting an opener
   that is a subframe).

This CL fixes this by:
1. Adding an |opener| parameter to WebContentsDelegate::ShouldCreateWebContents
2. Modifying constructor of BackgroundContents to take a new |opener|
   parameter and to pass it to WebContents::CreateParams.
3. Modifying WebContents::CreateWithSessionStorage so that it sets
   the opener on the newly created contents.
4. Changing WebContents::GetOpener and WebContents::GetOriginalOpener
   methods to return RenderFrameHost.

The accurate opener was originally needed for a planned but ultimately
abandoned UMA (see https://crbug.com/718516), but having an accurate
opener is desirable in general.

This CL re-enables AppBackgroundPageApiTest.OpenThenClose which was
disabled due to flakiness 4 years ago.  This test is where the
verification of the opener has to go.  I cannot repro the flakiness when
trying the test locally (i.e. running the test 30 times individually,
and 20 times as part of the whole AppBackgroundPageApiTest suite).

BUG=165644, 724280
==========

[Łukasz Anforowicz, 2017-05-22 17:19:27]

nick@, can you take another look please?

I've added extra test verification to AppBackgroundPageApiTest as you've
suggested.  To make the verification meaningful, I've also merged here another
CL that changes WebContents::GetOpener to return RenderFrameHost* (rather than
WebContents*).

https://codereview.chromium.org/2882513005/diff/60001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/2882513005/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:2625: opener, MSG_ROUTING_NONE, MSG_ROUTING_NONE,
MSG_ROUTING_NONE, profile_,
On 2017/05/17 20:45:27, ncarter (slow) wrote:
> this should be nullptr.
> 
> preserving the |opener| here is, I think incorrect: the opener doesn't get a
> reference to |contents| -- and I'm pretty sure window.opener in the background
> contents' document will be null.
> 

Good point - the new tests you've suggested would have failed without this. 
Done.

https://codereview.chromium.org/2882513005/diff/60001/components/offline_page...
File
components/offline_pages/content/background_loader/background_loader_contents_unittest.cc
(right):

https://codereview.chromium.org/2882513005/diff/60001/components/offline_page...
components/offline_pages/content/background_loader/background_loader_contents_unittest.cc:97:
TEST_F(BackgroundLoaderContentsTest, ShouldNotCreateWebContents) {
On 2017/05/17 20:45:27, ncarter (slow) wrote:
> Could you also add a few lines to app_background_page_apitest.cc to test the
> changes? The idea is to look for consistency between the nullness of
> |window.opener| in the background page, and the value of
> |WebContents::GetOpener()|.
> 
> Near the end of TEST_F(AppBackgroundPageApiTest, NoJsBackgroundPage):
> 
>  BackgroundContents* background_contents =
>
BackgroundContentsServiceFactory::GetForProfile(browser()->profile())->GetAppBackgroundContents(ASCIIToUTF16(extension->id());
> 
>  ASSERT_TRUE(background_contents);
>  ASSERT_FALSE(background_contents->web_contents()->GetOpener());
> 
> [and maybe, also, ExecuteScriptAndExtractBool(web_contents()->GetMainFrame(),
> "send(window.opener == null)"]

Good idea.  Done.
> 
> And do similar but opposite checks in a test w/ js access (is there even one
> there?)

Note that absence of allow_js_access defaults to true.

OTOH, the only default allow_js_access browser test that pauses after opening a
background page via window.open was disabled.  I cannot repro the flakiness
locally, so I am trying to reenable the test.

[Łukasz Anforowicz, 2017-05-22 17:21:33]

Description was changed from

==========
Propagate opener to BackgroundsContents.

Before this CL, the opener would be lost:
1. When creating the BackgroundContents:
    1.1. The call from WebContentsImpl::CreateNewWindow to
         Browser::ShouldCreateWebContents didn't propagate |opener|
    1.2. WebContents::CreateWithSessionStorage ignored opener-related
         fields in WebContents::CreateParams.
2. When inspecting the opener - WebContents::GetOpener used to return
   WebContents rather than RenderFrameHost (misrepresenting an opener
   that is a subframe).

This CL fixes this by:
1. Adding an |opener| parameter to WebContentsDelegate::ShouldCreateWebContents
2. Modifying constructor of BackgroundContents to take a new |opener|
   parameter and to pass it to WebContents::CreateParams.
3. Modifying WebContents::CreateWithSessionStorage so that it sets
   the opener on the newly created contents.
4. Changing WebContents::GetOpener and WebContents::GetOriginalOpener
   methods to return RenderFrameHost.

The accurate opener was originally needed for a planned but ultimately
abandoned UMA (see https://crbug.com/718516), but having an accurate
opener is desirable in general.

This CL re-enables AppBackgroundPageApiTest.OpenThenClose which was
disabled due to flakiness 4 years ago.  This test is where the
verification of the opener has to go.  I cannot repro the flakiness when
trying the test locally (i.e. running the test 30 times individually,
and 20 times as part of the whole AppBackgroundPageApiTest suite).

BUG=165644, 724280
==========

to

==========
Propagate opener to BackgroundsContents.

Before this CL, the opener would be lost:
1. When creating the BackgroundContents:
    1.1. The call from WebContentsImpl::CreateNewWindow to
         Browser::ShouldCreateWebContents didn't propagate |opener|
    1.2. WebContents::CreateWithSessionStorage ignored opener-related
         fields in WebContents::CreateParams.
2. When inspecting the opener - WebContents::GetOpener used to return
   WebContents rather than RenderFrameHost (misrepresenting an opener
   that is a subframe).

This CL fixes this by:
1. Adding an |opener| parameter to WebContentsDelegate::ShouldCreateWebContents
2. Modifying constructor of BackgroundContents to take a new |opener|
   parameter and to pass it to WebContents::CreateParams.
3. Modifying WebContents::CreateWithSessionStorage so that it sets
   the opener on the newly created contents.
4. Changing WebContents::GetOpener and WebContents::GetOriginalOpener
   methods to return RenderFrameHost.

The accurate opener was originally needed for a planned but ultimately
abandoned UMA (see https://crbug.com/718516), but having an accurate
opener is desirable in general.

This CL re-enables AppBackgroundPageApiTest.OpenThenClose which was
disabled due to flakiness 4 years ago.  This test is where the
verification of the opener has to go.  I cannot repro the flakiness when
trying the test locally (i.e. running the test 30 times individually,
and 20 times as part of the whole AppBackgroundPageApiTest suite) or on
the trybots (I've tried 3 x win_chromium_rel_ng, linux_chromium_rel_ng
and mac_chromium_rel_ng).

BUG=165644, 724280
==========

[ncarter (slow), 2017-05-22 17:32:43]

lgtm

https://codereview.chromium.org/2882513005/diff/80001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2882513005/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:183: ?
opener->GetLastCommittedURL()
I think this is an accuracy improvement, but let's make sure that
page_load_metrics OWNERS want it; this may move the metrics.

[Łukasz Anforowicz, 2017-05-22 18:25:37]

lukasza@chromium.org changed reviewers:
+ bmcquade@chromium.org

[Łukasz Anforowicz, 2017-05-22 18:25:38]

bmcquade@, could you PTAL at the changes in
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc?

The CL under review changes WebContents::GetOpener so that it returns the opener
*frame* (i.e. RenderFrameHost pointer) rather than the opener *contents* /
*page* (i.e. WebContents pointer).  This means that the opener is more accurate
and in general is a desirable change, but since this change can influence your
metrics, I wanted to give you a heads up and a chance to review the CL first. 
Please also see some notes inline/below.

https://codereview.chromium.org/2882513005/diff/80001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2882513005/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:183: ?
opener->GetLastCommittedURL()
On 2017/05/22 17:32:42, ncarter (slow) wrote:
> I think this is an accuracy improvement, but let's make sure that
> page_load_metrics OWNERS want it; this may move the metrics.

Good point.  Some notes about the change above:

- The change feeds into |currently_committed_url| below and then into the
constructor of PageLoadTracker.  The constructor doesn't store the URL anywhere
- it just passes the URL to PageLoadMetricsObserver::OnStart.  I've looked at
all the overloads of PageLoadMetricsObserver::OnStart and the URL is ignored in
all the overloads except FromGWSPageLoadMetricsLogger, where the URL influences
|previously_committed_url_is_search_results_| and
|previously_committed_url_is_search_redirector_| fields.  I assume making these
fields more accurate is okay.

- The comment on PageLoadMetricsObserver::OnStart says "currently_committed_url
contains the URL of the committed page load [...] or the empty URL".  This seems
inaccurate - it doesn't talk about the opener.

- AFAICT, the fields above feed into *FromGoogleSearch* metrics which are owned
by csharrison@ and bmcquade@

[Bryan McQuade, 2017-05-23 13:57:02]

Thanks!

https://codereview.chromium.org/2882513005/diff/80001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2882513005/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:183: ?
opener->GetLastCommittedURL()
On 2017/05/22 at 18:25:38, Łukasz A. wrote:
> On 2017/05/22 17:32:42, ncarter (slow) wrote:
> > I think this is an accuracy improvement, but let's make sure that
> > page_load_metrics OWNERS want it; this may move the metrics.
> 
> Good point.  Some notes about the change above:
> 
> - The change feeds into |currently_committed_url| below and then into the
constructor of PageLoadTracker.  The constructor doesn't store the URL anywhere
- it just passes the URL to PageLoadMetricsObserver::OnStart.  I've looked at
all the overloads of PageLoadMetricsObserver::OnStart and the URL is ignored in
all the overloads except FromGWSPageLoadMetricsLogger, where the URL influences
|previously_committed_url_is_search_results_| and
|previously_committed_url_is_search_redirector_| fields.  I assume making these
fields more accurate is okay.
> 
> - The comment on PageLoadMetricsObserver::OnStart says
"currently_committed_url contains the URL of the committed page load [...] or
the empty URL".  This seems inaccurate - it doesn't talk about the opener.
> 
> - AFAICT, the fields above feed into *FromGoogleSearch* metrics which are
owned by csharrison@ and bmcquade@

Thanks so much for the thorough analysis & reaching out to us to make sure this
is a change that makes sense for page load metrics.

You're right that the comment in OnStart() is out of date - it used to contain
exclusively the previously committed load's URL, but we updated it to also
handle the opener's URL, and just never fixed the comment. You are welcome to
update the comment as part of this CL if you like - happy to have the
improvement.

You're right that the FromGWSPageLoadMetricsLogger is the only component that's
affected by this change. I believe this change should be fine for that component
and potential other consumers of this field as well. I can watch the
FromGWSPageLoadMetricsLogger metrics after this change lands just to be sure we
don't see significant changes, and if we do, we can drill down to convince
ourselves there are no regressions.

Does the change here in page load metrics only affect computation of the opener
for background contents? Are there other potential changes in behavior that we
might see as a result of this change?

Overall I think I'm fine with this change, thanks for making it! And I really
appreciate you taking the time to do the analysis here and reach out to us for
the review. Just want to make sure I understand how this might change behavior
before we land.

Thanks!
-Bryan

[Łukasz Anforowicz, 2017-05-23 18:56:03]

https://codereview.chromium.org/2882513005/diff/80001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2882513005/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:183: ?
opener->GetLastCommittedURL()
On 2017/05/23 13:57:02, Bryan McQuade wrote:
> On 2017/05/22 at 18:25:38, Łukasz A. wrote:
> > On 2017/05/22 17:32:42, ncarter (slow) wrote:
> > > I think this is an accuracy improvement, but let's make sure that
> > > page_load_metrics OWNERS want it; this may move the metrics.
> > 
> > Good point.  Some notes about the change above:
> > 
> > - The change feeds into |currently_committed_url| below and then into the
> constructor of PageLoadTracker.  The constructor doesn't store the URL
anywhere
> - it just passes the URL to PageLoadMetricsObserver::OnStart.  I've looked at
> all the overloads of PageLoadMetricsObserver::OnStart and the URL is ignored
in
> all the overloads except FromGWSPageLoadMetricsLogger, where the URL
influences
> |previously_committed_url_is_search_results_| and
> |previously_committed_url_is_search_redirector_| fields.  I assume making
these
> fields more accurate is okay.
> > 
> > - The comment on PageLoadMetricsObserver::OnStart says
> "currently_committed_url contains the URL of the committed page load [...] or
> the empty URL".  This seems inaccurate - it doesn't talk about the opener.
> > 
> > - AFAICT, the fields above feed into *FromGoogleSearch* metrics which are
> owned by csharrison@ and bmcquade@
> 
> Thanks so much for the thorough analysis & reaching out to us to make sure
this
> is a change that makes sense for page load metrics.
> 
> You're right that the comment in OnStart() is out of date - it used to contain
> exclusively the previously committed load's URL, but we updated it to also
> handle the opener's URL, and just never fixed the comment. You are welcome to
> update the comment as part of this CL if you like - happy to have the
> improvement.
> 
> You're right that the FromGWSPageLoadMetricsLogger is the only component
that's
> affected by this change. I believe this change should be fine for that
component
> and potential other consumers of this field as well. I can watch the
> FromGWSPageLoadMetricsLogger metrics after this change lands just to be sure
we
> don't see significant changes, and if we do, we can drill down to convince
> ourselves there are no regressions.
> 
> Does the change here in page load metrics only affect computation of the
opener
> for background contents? Are there other potential changes in behavior that we
> might see as a result of this change?

The change of return type of WebContents::GetOpener affects any WebContents, not
necessarily background contents (changing WebContents::GetOpener is necessary
for meaningful/accurate test coverage for the background contents change).

For the metrics here, there should be no change if the opener is a main frame
(because some_web_contents->GetLastCommittedURL() returns the same URL as
some_web_contents->GetMainFrame()->GetLastCommittedURL()).  But, if the opener
is a subframe, then the URL calculated for FromGWSPageLoadMetricsLogger will
change (before - it would be the URL of the main/top-level frame, after - it
would be the URL of the actual opener frame).

So - I think that the impact to the metrics depends on: 
1) Whether the opener (VS commited_load_) path is taken below on line 185.
2) Whether the opener is a subframe
3) Whether FromGWSPageLoadMetricsLogger::IsGoogleSearchResultUrl and/or
IsGoogleSearchRedirectorUrl would return different results for the main frame vs
the opener subframe.

> Overall I think I'm fine with this change, thanks for making it! And I really
> appreciate you taking the time to do the analysis here and reach out to us for
> the review. Just want to make sure I understand how this might change behavior
> before we land.

[Bryan McQuade, 2017-05-23 19:06:40]

lgtm

https://codereview.chromium.org/2882513005/diff/80001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2882513005/diff/80001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:183: ?
opener->GetLastCommittedURL()
On 2017/05/23 at 18:56:03, Łukasz A. wrote:
> On 2017/05/23 13:57:02, Bryan McQuade wrote:
> > On 2017/05/22 at 18:25:38, Łukasz A. wrote:
> > > On 2017/05/22 17:32:42, ncarter (slow) wrote:
> > > > I think this is an accuracy improvement, but let's make sure that
> > > > page_load_metrics OWNERS want it; this may move the metrics.
> > > 
> > > Good point.  Some notes about the change above:
> > > 
> > > - The change feeds into |currently_committed_url| below and then into the
> > constructor of PageLoadTracker.  The constructor doesn't store the URL
anywhere
> > - it just passes the URL to PageLoadMetricsObserver::OnStart.  I've looked
at
> > all the overloads of PageLoadMetricsObserver::OnStart and the URL is ignored
in
> > all the overloads except FromGWSPageLoadMetricsLogger, where the URL
influences
> > |previously_committed_url_is_search_results_| and
> > |previously_committed_url_is_search_redirector_| fields.  I assume making
these
> > fields more accurate is okay.
> > > 
> > > - The comment on PageLoadMetricsObserver::OnStart says
> > "currently_committed_url contains the URL of the committed page load [...]
or
> > the empty URL".  This seems inaccurate - it doesn't talk about the opener.
> > > 
> > > - AFAICT, the fields above feed into *FromGoogleSearch* metrics which are
> > owned by csharrison@ and bmcquade@
> > 
> > Thanks so much for the thorough analysis & reaching out to us to make sure
this
> > is a change that makes sense for page load metrics.
> > 
> > You're right that the comment in OnStart() is out of date - it used to
contain
> > exclusively the previously committed load's URL, but we updated it to also
> > handle the opener's URL, and just never fixed the comment. You are welcome
to
> > update the comment as part of this CL if you like - happy to have the
> > improvement.
> > 
> > You're right that the FromGWSPageLoadMetricsLogger is the only component
that's
> > affected by this change. I believe this change should be fine for that
component
> > and potential other consumers of this field as well. I can watch the
> > FromGWSPageLoadMetricsLogger metrics after this change lands just to be sure
we
> > don't see significant changes, and if we do, we can drill down to convince
> > ourselves there are no regressions.
> > 
> > Does the change here in page load metrics only affect computation of the
opener
> > for background contents? Are there other potential changes in behavior that
we
> > might see as a result of this change?
> 
> The change of return type of WebContents::GetOpener affects any WebContents,
not necessarily background contents (changing WebContents::GetOpener is
necessary for meaningful/accurate test coverage for the background contents
change).
> 
> For the metrics here, there should be no change if the opener is a main frame
(because some_web_contents->GetLastCommittedURL() returns the same URL as
some_web_contents->GetMainFrame()->GetLastCommittedURL()).  But, if the opener
is a subframe, then the URL calculated for FromGWSPageLoadMetricsLogger will
change (before - it would be the URL of the main/top-level frame, after - it
would be the URL of the actual opener frame).
> 
> So - I think that the impact to the metrics depends on: 
> 1) Whether the opener (VS commited_load_) path is taken below on line 185.
> 2) Whether the opener is a subframe
> 3) Whether FromGWSPageLoadMetricsLogger::IsGoogleSearchResultUrl and/or
IsGoogleSearchRedirectorUrl would return different results for the main frame vs
the opener subframe.
> 
> > Overall I think I'm fine with this change, thanks for making it! And I
really
> > appreciate you taking the time to do the analysis here and reach out to us
for
> > the review. Just want to make sure I understand how this might change
behavior
> > before we land.

Thanks! I see now. This change SGTM, thanks!

[Łukasz Anforowicz, 2017-05-23 20:55:56]

lukasza@chromium.org changed reviewers:
+ bauerb@chromium.org, boliu@chromium.org, dimich@chromium.org,
droger@chromium.org, hubbe@chromium.org, lfg@chromium.org, sadrul@chromium.org

[Łukasz Anforowicz, 2017-05-23 20:55:56]

+bauerb@ for chrome/browser/android
+hubbe@ for chrome/browser/extensions/api/tab_capture
+droger@ for chrome/browser/prerender
+dimich@ for components/offline_pages
+boliu@ for components/web_contents_delegate_android
+lfg@ for extensions/browser/guest_view/extension_options
+sadrul@ for ui/keyboard and ui/views/controls/webview

This CL adds a new parameter to WebContentsDelegate::ShouldCreateWebContents -
the files above (the ones I am asking you to review above) simply ignore the new
parameter.

[boliu, 2017-05-23 21:19:43]

On 2017/05/23 20:55:56, Łukasz A. wrote:
> +bauerb@ for chrome/browser/android
> +hubbe@ for chrome/browser/extensions/api/tab_capture
> +droger@ for chrome/browser/prerender
> +dimich@ for components/offline_pages
> +boliu@ for components/web_contents_delegate_android

rs lgtm

> +lfg@ for extensions/browser/guest_view/extension_options
> +sadrul@ for ui/keyboard and ui/views/controls/webview
> 
> This CL adds a new parameter to WebContentsDelegate::ShouldCreateWebContents -
> the files above (the ones I am asking you to review above) simply ignore the
new
> parameter.

[Bernhard Bauer, 2017-05-24 09:34:47]

Android LGTM.

[lfg, 2017-05-24 14:46:51]

guest_view lgtm

[Dmitry Titov, 2017-05-24 18:07:52]

offline_pages lgtm

[hubbe, 2017-05-24 18:47:08]

tab_capture LGTM

[sadrul, 2017-05-25 04:05:51]

lgtm

[Łukasz Anforowicz, 2017-05-25 04:27:35]

lukasza@chromium.org changed reviewers:
+ davidben@chromium.org
- droger@chromium.org

[Łukasz Anforowicz, 2017-05-25 04:27:36]

davidben@, could you PTAL at chrome/browser/prerender (since droger@ is OOO)?

This CL adds a new parameter to WebContentsDelegate::ShouldCreateWebContents -
the overrides in chrome/browser/prerender simply ignore the new parameter.

[davidben, 2017-05-25 17:48:22]

lgtm

[Łukasz Anforowicz, 2017-05-25 18:05:25]

lukasza@chromium.org changed reviewers:
+ atwilson@chromium.org

[Łukasz Anforowicz, 2017-05-25 18:05:26]

atwilson@, could you PTAL at chrome/browser/background as well as
chrome/browser/extensions/app_background_page_apitest.cc and
chrome/browser/chromeos/first_run/drive_first_run_controller.cc (even though you
are not an owner of the last 2, they are closely related to background contents
[!= background pages! :-)]).

[Andrew T Wilson (Slow), 2017-06-05 08:32:46]

LGTM, although I did have one question about why we're passing in nullptr
instead of opener in one case.

I presume the code is correct since the tests pass, which is why I'm giving the
LGTM - if the code isn't correct, then please add a test to test the case that
was broken.

https://codereview.chromium.org/2882513005/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/drive_first_run_controller.cc (right):

https://codereview.chromium.org/2882513005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/drive_first_run_controller.cc:315:
content::SiteInstance::Create(profile_), nullptr, MSG_ROUTING_NONE,
I'm not sure why we are passing in nullptr here instead of opener - maybe this
is worth a comment?

[Andrew T Wilson (Slow), 2017-06-05 08:32:46]

LGTM, although I did have one question about why we're passing in nullptr
instead of opener in one case.

I presume the code is correct since the tests pass, which is why I'm giving the
LGTM - if the code isn't correct, then please add a test to test the case that
was broken.

https://codereview.chromium.org/2882513005/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/drive_first_run_controller.cc (right):

https://codereview.chromium.org/2882513005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/drive_first_run_controller.cc:315:
content::SiteInstance::Create(profile_), nullptr, MSG_ROUTING_NONE,
I'm not sure why we are passing in nullptr here instead of opener - maybe this
is worth a comment?

[Łukasz Anforowicz, 2017-06-05 16:57:57]

On 2017/06/05 08:32:46, Andrew T Wilson (Slow) wrote:
> LGTM, although I did have one question about why we're passing in nullptr
> instead of opener in one case.

Okay - thanks.
> 
> I presume the code is correct since the tests pass, which is why I'm giving
the
> LGTM - if the code isn't correct, then please add a test to test the case that
> was broken.

The code should be correct, but you're right that this is not obvious - I've
tweaked the comment.

https://codereview.chromium.org/2882513005/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/drive_first_run_controller.cc (right):

https://codereview.chromium.org/2882513005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/drive_first_run_controller.cc:315:
content::SiteInstance::Create(profile_), nullptr, MSG_ROUTING_NONE,
On 2017/06/05 08:32:46, Andrew T Wilson (Slow) wrote:
> I'm not sure why we are passing in nullptr here instead of opener - maybe this
> is worth a comment?

Good point - this is not that obvious what is going on here.  If you look at
Browser::MaybeCreateBackgroundContents, then it has 2 calls to
CreateBackgroundContents - one where the opener is propagated, and one where no
opener is propagated (and where MSG_ROUTING_NONE are passed, because [since
we're not establishing an opener relationship] we don't care to create the new
contents in the same renderer process as the opener).  Here we also need the
second case, because (just like the second case in
Browser::MaybeCreateBackgroundContents) here also we have allow_js_access:false
(i.e. see chrome/test/data/drive_first_run/app/manifest.json) that corresponds
to the MSG_ROUTING_NONE usage.

I've added a comment.

[Łukasz Anforowicz, 2017-06-05 17:00:46]

lukasza@chromium.org changed reviewers:
+ sky@chromium.org

[Łukasz Anforowicz, 2017-06-05 17:00:47]

sky@, could you please do an owners review for //chrome?  Most things have
already been reviewed by owners of more specific directories, but the following
4 files still need an owner approval:
    chrome/browser/chromeos/first_run/drive_first_run_controller.cc
    chrome/browser/extensions/app_background_page_apitest.cc
    chrome/browser/ui/browser.cc
    chrome/browser/ui/browser.h

[sky, 2017-06-05 20:01:30]

Said files LGTM

[Łukasz Anforowicz, 2017-06-05 20:08:31]

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

[commit-bot: I haz the power, 2017-06-05 20:09:33]

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-06-06 00:36:22]

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

[commit-bot: I haz the power, 2017-06-06 00:36:23]

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

[Łukasz Anforowicz, 2017-06-06 01:07:58]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2017-06-06 01:07:59]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
nick@chromium.org, sadrul@chromium.org, hubbe@chromium.org,
atwilson@chromium.org, dimich@chromium.org, davidben@chromium.org,
boliu@chromium.org, bmcquade@chromium.org, lfg@chromium.org
Link to the patchset: https://codereview.chromium.org/2882513005/#ps100001
(title: "Tweaked the comment in
DriveWebContentsManager::ShouldCreateWebContents")

[commit-bot: I haz the power, 2017-06-06 01:08:33]

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-06-06 03:10:27]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496711278743640,
"parent_rev": "05729e73de37e9f74e80e1009350422142b7aae5", "commit_rev":
"6f8ac625db8fc02373327a9957e6633b62717d0a"}

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

Description was changed from

==========
Propagate opener to BackgroundsContents.

Before this CL, the opener would be lost:
1. When creating the BackgroundContents:
    1.1. The call from WebContentsImpl::CreateNewWindow to
         Browser::ShouldCreateWebContents didn't propagate |opener|
    1.2. WebContents::CreateWithSessionStorage ignored opener-related
         fields in WebContents::CreateParams.
2. When inspecting the opener - WebContents::GetOpener used to return
   WebContents rather than RenderFrameHost (misrepresenting an opener
   that is a subframe).

This CL fixes this by:
1. Adding an |opener| parameter to WebContentsDelegate::ShouldCreateWebContents
2. Modifying constructor of BackgroundContents to take a new |opener|
   parameter and to pass it to WebContents::CreateParams.
3. Modifying WebContents::CreateWithSessionStorage so that it sets
   the opener on the newly created contents.
4. Changing WebContents::GetOpener and WebContents::GetOriginalOpener
   methods to return RenderFrameHost.

The accurate opener was originally needed for a planned but ultimately
abandoned UMA (see https://crbug.com/718516), but having an accurate
opener is desirable in general.

This CL re-enables AppBackgroundPageApiTest.OpenThenClose which was
disabled due to flakiness 4 years ago.  This test is where the
verification of the opener has to go.  I cannot repro the flakiness when
trying the test locally (i.e. running the test 30 times individually,
and 20 times as part of the whole AppBackgroundPageApiTest suite) or on
the trybots (I've tried 3 x win_chromium_rel_ng, linux_chromium_rel_ng
and mac_chromium_rel_ng).

BUG=165644, 724280
==========

to

==========
Propagate opener to BackgroundsContents.

Before this CL, the opener would be lost:
1. When creating the BackgroundContents:
    1.1. The call from WebContentsImpl::CreateNewWindow to
         Browser::ShouldCreateWebContents didn't propagate |opener|
    1.2. WebContents::CreateWithSessionStorage ignored opener-related
         fields in WebContents::CreateParams.
2. When inspecting the opener - WebContents::GetOpener used to return
   WebContents rather than RenderFrameHost (misrepresenting an opener
   that is a subframe).

This CL fixes this by:
1. Adding an |opener| parameter to WebContentsDelegate::ShouldCreateWebContents
2. Modifying constructor of BackgroundContents to take a new |opener|
   parameter and to pass it to WebContents::CreateParams.
3. Modifying WebContents::CreateWithSessionStorage so that it sets
   the opener on the newly created contents.
4. Changing WebContents::GetOpener and WebContents::GetOriginalOpener
   methods to return RenderFrameHost.

The accurate opener was originally needed for a planned but ultimately
abandoned UMA (see https://crbug.com/718516), but having an accurate
opener is desirable in general.

This CL re-enables AppBackgroundPageApiTest.OpenThenClose which was
disabled due to flakiness 4 years ago.  This test is where the
verification of the opener has to go.  I cannot repro the flakiness when
trying the test locally (i.e. running the test 30 times individually,
and 20 times as part of the whole AppBackgroundPageApiTest suite) or on
the trybots (I've tried 3 x win_chromium_rel_ng, linux_chromium_rel_ng
and mac_chromium_rel_ng).

BUG=165644, 724280

Review-Url: https://codereview.chromium.org/2882513005
Cr-Commit-Position: refs/heads/master@{#477169}
Committed:
https://chromium.googlesource.com/chromium/src/+/6f8ac625db8fc02373327a9957e6...
==========

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

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/6f8ac625db8fc02373327a9957e6...