Propagate opener to BackgroundsContents.

chrome/browser/page_load_metrics/metrics_web_contents_observer.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/page_load_metrics/metrics_web_contents_observer.h"
 
 #include <algorithm>
 #include <ostream>
 #include <string>
 #include <utility>
@@ skipping 160 matching lines @@
   }
 
   if (!ShouldTrackNavigation(navigation_handle))
     return;
 
   // Pass in the last committed url to the PageLoadTracker. If the MWCO has
   // never observed a committed load, use the last committed url from this
   // WebContent's opener. This is more accurate than using referrers due to
   // referrer sanitizing and origin referrers. Note that this could potentially
   // be inaccurate if the opener has since navigated.
-  content::WebContents* opener = web_contents()->GetOpener();
-  const GURL& opener_url =
-      !has_navigated_ && opener
-          ? web_contents()->GetOpener()->GetLastCommittedURL()
-          : GURL::EmptyGURL();
+  content::RenderFrameHost* opener = web_contents()->GetOpener();
+  const GURL& opener_url = !has_navigated_ && opener
+                               ? opener->GetLastCommittedURL()

[ncarter (slow), 2017/05/22 17:32:42]

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:38]

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

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.

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

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

+                               : GURL::EmptyGURL();
   const GURL& currently_committed_url =
       committed_load_ ? committed_load_->url() : opener_url;
   has_navigated_ = true;
 
   // We can have two provisional loads in some cases. E.g. a same-site
   // navigation can have a concurrent cross-process navigation started
   // from the omnibox.
   DCHECK_GT(2ul, provisional_loads_.size());
   // Passing raw pointers to observers_ and embedder_interface_ is safe because
   // the MetricsWebContentsObserver owns them both list and they are torn down
@@ skipping 446 matching lines @@
     observer_->RemoveTestingObserver(this);
     observer_ = nullptr;
   }
 }
 
 void MetricsWebContentsObserver::TestingObserver::OnGoingAway() {
   observer_ = nullptr;
 }
 
 }  // namespace page_load_metrics