Make PageLoadMetricsWaiter more resilient

chrome/browser/page_load_metrics/page_load_metrics_observer.h

 // 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.
 
 #ifndef CHROME_BROWSER_PAGE_LOAD_METRICS_PAGE_LOAD_METRICS_OBSERVER_H_
 #define CHROME_BROWSER_PAGE_LOAD_METRICS_PAGE_LOAD_METRICS_OBSERVER_H_
 
 #include <memory>
 #include <string>
 
@@ skipping 318 matching lines @@
   // default is to observe HTML and XHTML only. Note that PageLoadTrackers only
   // track XHTML, HTML, and MHTML (related/multipart).
   virtual ObservePolicy ShouldObserveMimeType(
       const std::string& mime_type) const;
 
   // The callbacks below are only invoked after a navigation commits, for
   // tracked page loads. Page loads that don't meet the criteria for being
   // tracked at the time a navigation commits will not receive any of the
   // callbacks below.
 
-  // OnTimingUpdate is triggered when an updated PageLoadTiming is
-  // available. This method may be called multiple times over the course of the
-  // page load. This method is currently only intended for use in testing. Most
-  // implementers should implement one of the On* callbacks, such as
-  // OnFirstContentfulPaint or OnDomContentLoadedEventStart. Please email
-  // loading-dev@chromium.org if you intend to override this method.
-  virtual void OnTimingUpdate(const mojom::PageLoadTiming& timing,
+  // OnTimingUpdate is triggered when an updated PageLoadTiming is available at
+  // the page or subframe level. This method may be called multiple times over

[Charlie Harrison, 2017/05/25 19:35:34]

Aside: It's weird to me to distinguish "page" vs "subframe" when usually we're
comparing main frame vs subframe. I think we really do want that distinction
because for "page" we aggregate, is that intuition correct?

[Bryan McQuade, 2017/05/25 19:54:06]

Correct - page is essentially main frame + aggregated data for a few page-level
fields (paint timings). I agree this isn't super clear. I added a comment to
explain.

+  // the course of the page load. This method is currently only intended for use
+  // in testing. Most implementers should implement one of the On* callbacks,
+  // such as OnFirstContentfulPaint or OnDomContentLoadedEventStart. Please
+  // email loading-dev@chromium.org if you intend to override this method.
+  virtual void OnTimingUpdate(bool is_subframe,

[Charlie Harrison, 2017/05/25 19:35:34]

optional: For consistency would it make sense to have two methods OnSubframe...
and OnMainFrame... like how we do it in the dispatcher client?

[Bryan McQuade, 2017/05/25 19:54:06]

I did this initially but decided that since this was really intended only for
testing, keeping the testing API thin was beneficial.

[Charlie Harrison, 2017/05/25 19:59:44]

Makes sense to me

+                              const mojom::PageLoadTiming& timing,
                               const PageLoadExtraInfo& extra_info) {}
 
   // OnUserInput is triggered when a new user input is passed in to
   // web_contents. Contains a TimeDelta from navigation start.
   virtual void OnUserInput(const blink::WebInputEvent& event) {}
 
   // The following methods are invoked at most once, when the timing for the
   // associated event first becomes available.
   virtual void OnDomContentLoadedEventStart(
       const mojom::PageLoadTiming& timing,
@@ skipping 88 matching lines @@
   // Called whenever a request is loaded for this page load. This comes
   // unfiltered from the ResourceDispatcherHost and may include blob requests
   // and data uris.
   virtual void OnLoadedResource(
       const ExtraRequestCompleteInfo& extra_request_complete_info) {}
 };
 
 }  // namespace page_load_metrics
 
 #endif  // CHROME_BROWSER_PAGE_LOAD_METRICS_PAGE_LOAD_METRICS_OBSERVER_H_