Refactor PageLoadMetricsObserver completion callback policy

Refactor PageLoadMetricsObserver completion callback policy

This patch modifies PageLoadMetricsObserver to invoke OnComplete for tracked
loads that committed, and OnFailedProvisionalLoad for tracked loads that did
not commit.

This makes it more straightforward for implementers to reason about which method
to override, and encourages separation of code for the different states into
methods which makes observer code more maintainable.

This patch also refactors the various observers to implement these methods as
needed.

BUG=627585
Committed: https://crrev.com/0259d67099838b4b0f2aabcbd3108e9952d56204
Cr-Commit-Position: refs/heads/master@{#406454}

[Bryan McQuade, 2016-07-14 21:03:02]

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

[commit-bot: I haz the power, 2016-07-14 21:03:45]

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, 2016-07-14 22:21:46]

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

[commit-bot: I haz the power, 2016-07-14 22:21:47]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2016-07-15 12:31:27]

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

[commit-bot: I haz the power, 2016-07-15 12:32:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2016-07-15 12:37:59]

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

[commit-bot: I haz the power, 2016-07-15 12:38:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2016-07-15 12:56:48]

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

[commit-bot: I haz the power, 2016-07-15 12:57:02]

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, 2016-07-15 13:58:09]

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

[commit-bot: I haz the power, 2016-07-15 13:58:10]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2016-07-15 14:05:31]

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

[commit-bot: I haz the power, 2016-07-15 14:05:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2016-07-15 14:20:32]

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

[commit-bot: I haz the power, 2016-07-15 14:20:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2016-07-15 14:22:46]

Description was changed from

==========
update complete callback api

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

to

==========
Modify PageLoadMetricsObserver to invoke OnComplete for tracked loads that
committed, and OnFailedProvisionalLoad for tracked loads that did not commit.

This makes it more straightforward for implementers to reason about which method
to override, and encourages separation of code for the different states into
methods which makes observer code more maintainable.

This patch also refactors the various observers to implement these methods as
needed.

BUG=627585
==========

[Bryan McQuade, 2016-07-15 14:23:44]

Description was changed from

==========
Modify PageLoadMetricsObserver to invoke OnComplete for tracked loads that
committed, and OnFailedProvisionalLoad for tracked loads that did not commit.

This makes it more straightforward for implementers to reason about which method
to override, and encourages separation of code for the different states into
methods which makes observer code more maintainable.

This patch also refactors the various observers to implement these methods as
needed.

BUG=627585
==========

to

==========
Clean up PLMO completion callback policy

This patch modifies PageLoadMetricsObserver to invoke OnComplete for tracked
loads that committed, and OnFailedProvisionalLoad for tracked loads that did
not commit.

This makes it more straightforward for implementers to reason about which method
to override, and encourages separation of code for the different states into
methods which makes observer code more maintainable.

This patch also refactors the various observers to implement these methods as
needed.

BUG=627585
==========

[Bryan McQuade, 2016-07-15 14:32:31]

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

[commit-bot: I haz the power, 2016-07-15 14:32:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2016-07-15 14:36:55]

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

[commit-bot: I haz the power, 2016-07-15 14:37: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, 2016-07-15 14:38:14]

Description was changed from

==========
Clean up PLMO completion callback policy

This patch modifies PageLoadMetricsObserver to invoke OnComplete for tracked
loads that committed, and OnFailedProvisionalLoad for tracked loads that did
not commit.

This makes it more straightforward for implementers to reason about which method
to override, and encourages separation of code for the different states into
methods which makes observer code more maintainable.

This patch also refactors the various observers to implement these methods as
needed.

BUG=627585
==========

to

==========
Refactor PLMO completion callback policy

This patch modifies PageLoadMetricsObserver to invoke OnComplete for tracked
loads that committed, and OnFailedProvisionalLoad for tracked loads that did
not commit.

This makes it more straightforward for implementers to reason about which method
to override, and encourages separation of code for the different states into
methods which makes observer code more maintainable.

This patch also refactors the various observers to implement these methods as
needed.

BUG=627585
==========

[Bryan McQuade, 2016-07-15 14:40:48]

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

[commit-bot: I haz the power, 2016-07-15 14:41:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2016-07-15 14:51:05]

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

[commit-bot: I haz the power, 2016-07-15 14:51:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2016-07-15 15:01:26]

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

[commit-bot: I haz the power, 2016-07-15 15:02:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2016-07-15 15:03:22]

Description was changed from

==========
Refactor PLMO completion callback policy

This patch modifies PageLoadMetricsObserver to invoke OnComplete for tracked
loads that committed, and OnFailedProvisionalLoad for tracked loads that did
not commit.

This makes it more straightforward for implementers to reason about which method
to override, and encourages separation of code for the different states into
methods which makes observer code more maintainable.

This patch also refactors the various observers to implement these methods as
needed.

BUG=627585
==========

to

==========
Refactor PageLoadMetricsObserver completion callback policy

This patch modifies PageLoadMetricsObserver to invoke OnComplete for tracked
loads that committed, and OnFailedProvisionalLoad for tracked loads that did
not commit.

This makes it more straightforward for implementers to reason about which method
to override, and encourages separation of code for the different states into
methods which makes observer code more maintainable.

This patch also refactors the various observers to implement these methods as
needed.

BUG=627585
==========

[Bryan McQuade, 2016-07-15 15:30:52]

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

[commit-bot: I haz the power, 2016-07-15 15:31:45]

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, 2016-07-15 16:34:32]

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

[commit-bot: I haz the power, 2016-07-15 16:34:33]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2016-07-15 17:46:14]

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

[commit-bot: I haz the power, 2016-07-15 17:46:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2016-07-15 17:48:21]

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

[Bryan McQuade, 2016-07-15 17:49:06]

Here's the follow up to the earlier change which changes OnComplete callback
policy to invoke OnComplete for committed tracked loads, and
OnFailedProvisionalLoad for tracked loads that failed to commit.

[commit-bot: I haz the power, 2016-07-15 18:55:29]

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

[commit-bot: I haz the power, 2016-07-15 18:55:30]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2016-07-17 13:47:58]

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

[commit-bot: I haz the power, 2016-07-17 13:48: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, 2016-07-17 14:55:43]

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

[commit-bot: I haz the power, 2016-07-17 14:55:44]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Charlie Harrison, 2016-07-18 20:02:18]

Looks good!

https://codereview.chromium.org/2152683004/diff/250001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2152683004/diff/250001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:435:
PAGE_LOAD_HISTOGRAM(internal::kHistogramBackgroundBeforeCommit,
Since we call this twice now, can you bump it into a static method / function so
we don't inline the PAGE_LOAD_HISTOGRAM macro twice?

https://codereview.chromium.org/2152683004/diff/250001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc
(left):

https://codereview.chromium.org/2152683004/diff/250001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:178:
if (!info.time_to_commit || timing.IsEmpty())
Why remove the IsEmpty() check?

https://codereview.chromium.org/2152683004/diff/250001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc
(left):

https://codereview.chromium.org/2152683004/diff/250001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc:850:
TEST_F(FromGWSPageLoadMetricsLoggerTest, ProvisionalNonHttpOrHttpsScheme) {
Looks like the test you added doesn't cover the same cases as these deleted
tests.

https://codereview.chromium.org/2152683004/diff/250001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/2152683004/diff/250001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:230:
base::Optional<FailedProvisionalLoadInfo> failed_provisional_load_info_;
Can this be a unique_ptr? If we never fail then we don't need to allocate memory
for a whole FailedProvisionalLoadInfo.

https://codereview.chromium.org/2152683004/diff/250001/components/page_load_m...
File components/page_load_metrics/browser/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2152683004/diff/250001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_observer.h:54: struct
FailedProvisionalLoadInfo {
nit: Can members be declared after methods? That's how PageLoadExtraInfo is set
up.

[Bryan McQuade, 2016-07-19 12:36:59]

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

[commit-bot: I haz the power, 2016-07-19 12:37:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2016-07-19 12:53:50]

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

[commit-bot: I haz the power, 2016-07-19 12:54:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2016-07-19 12:55:39]

Thanks! I addressed comments. PTAL.

https://codereview.chromium.org/2152683004/diff/250001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2152683004/diff/250001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:435:
PAGE_LOAD_HISTOGRAM(internal::kHistogramBackgroundBeforeCommit,
On 2016/07/18 at 20:02:18, csharrison wrote:
> Since we call this twice now, can you bump it into a static method / function
so we don't inline the PAGE_LOAD_HISTOGRAM macro twice?

I think we only log the background before commit histogram once, when we
encounter a failed provisional load. Which bit of code were you suggesting
factoring into a helper?

Note that we have NavigationToFirstForeground/Background histograms as well, but
those were only ever called for committed loads, so we only need to include them
in OnComplete.

https://codereview.chromium.org/2152683004/diff/250001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc
(left):

https://codereview.chromium.org/2152683004/diff/250001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:178:
if (!info.time_to_commit || timing.IsEmpty())
On 2016/07/18 at 20:02:18, csharrison wrote:
> Why remove the IsEmpty() check?

Yeah, you're right, we might as well return early here if timing is empty. Done.

https://codereview.chromium.org/2152683004/diff/250001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc
(left):

https://codereview.chromium.org/2152683004/diff/250001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc:850:
TEST_F(FromGWSPageLoadMetricsLoggerTest, ProvisionalNonHttpOrHttpsScheme) {
On 2016/07/18 at 20:02:18, csharrison wrote:
> Looks like the test you added doesn't cover the same cases as these deleted
tests.

Ah, you mean it not being a provisional load? I added a provisional version of
the new test as well. I think the 2 intent:-based tests I removed were redundant
- they were both just testing different intent: URLs. So I didn't see a need to
replicate both of those provisional tests. I agree it's be good to have a
provisional version of the test so I added that. I also verified that if I
change the intent: URL to an https: URL, both tests fail, so these tests are
verifying that we ignore intent-based navigations.

https://codereview.chromium.org/2152683004/diff/250001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/2152683004/diff/250001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.h:230:
base::Optional<FailedProvisionalLoadInfo> failed_provisional_load_info_;
On 2016/07/18 at 20:02:18, csharrison wrote:
> Can this be a unique_ptr? If we never fail then we don't need to allocate
memory for a whole FailedProvisionalLoadInfo.

Good idea, done.

https://codereview.chromium.org/2152683004/diff/250001/components/page_load_m...
File components/page_load_metrics/browser/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2152683004/diff/250001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_observer.h:54: struct
FailedProvisionalLoadInfo {
On 2016/07/18 at 20:02:18, csharrison wrote:
> nit: Can members be declared after methods? That's how PageLoadExtraInfo is
set up.

Ah, you're right, I think constructors go before members according to style
guide. Fixed, thanks!

[Bryan McQuade, 2016-07-19 12:57:27]

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

[Bryan McQuade, 2016-07-19 12:57:38]

bmcquade@chromium.org changed reviewers:
- isherman@chromium.org

[Bryan McQuade, 2016-07-19 12:57:39]

isherman PTAL for histograms.xml, thanks!

[Charlie Harrison, 2016-07-19 13:26:03]

This lgtm now. Thanks!

https://codereview.chromium.org/2152683004/diff/250001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2152683004/diff/250001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:435:
PAGE_LOAD_HISTOGRAM(internal::kHistogramBackgroundBeforeCommit,
On 2016/07/19 12:55:38, Bryan McQuade wrote:
> On 2016/07/18 at 20:02:18, csharrison wrote:
> > Since we call this twice now, can you bump it into a static method /
function
> so we don't inline the PAGE_LOAD_HISTOGRAM macro twice?
> 
> I think we only log the background before commit histogram once, when we
> encounter a failed provisional load. Which bit of code were you suggesting
> factoring into a helper?
> 
> Note that we have NavigationToFirstForeground/Background histograms as well,
but
> those were only ever called for committed loads, so we only need to include
them
> in OnComplete.

Ah you're right, I was misreading the histogram names.

[commit-bot: I haz the power, 2016-07-19 13:43:30]

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

[commit-bot: I haz the power, 2016-07-19 13:43:31]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2016-07-19 16:27:23]

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

[commit-bot: I haz the power, 2016-07-19 16:27:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2016-07-19 16:38:16]

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

[Bryan McQuade, 2016-07-19 16:38:52]

isherman PTAL for histograms.xml, thanks!

(adding isherman for real this time)

[commit-bot: I haz the power, 2016-07-19 17:34:34]

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

[commit-bot: I haz the power, 2016-07-19 17:34:35]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2016-07-19 20:11:19]

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

[commit-bot: I haz the power, 2016-07-19 20:12:12]

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, 2016-07-19 20:17:30]

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

[commit-bot: I haz the power, 2016-07-19 20:17:32]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2016-07-20 01:49:32]

histograms.xml lgtm, thanks

[Ilya Sherman, 2016-07-20 01:49:40]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2016-07-20 01:49:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2152683004/#ps310001
(title: "remove histogram checks that can be flaky due to immediate logging")

[commit-bot: I haz the power, 2016-07-20 01:50:37]

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, 2016-07-20 01:56:32]

Committed patchset #17 (id:310001)

[commit-bot: I haz the power, 2016-07-20 01:56:36]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-20 01:57:48]

Description was changed from

==========
Refactor PageLoadMetricsObserver completion callback policy

This patch modifies PageLoadMetricsObserver to invoke OnComplete for tracked
loads that committed, and OnFailedProvisionalLoad for tracked loads that did
not commit.

This makes it more straightforward for implementers to reason about which method
to override, and encourages separation of code for the different states into
methods which makes observer code more maintainable.

This patch also refactors the various observers to implement these methods as
needed.

BUG=627585
==========

to

==========
Refactor PageLoadMetricsObserver completion callback policy

This patch modifies PageLoadMetricsObserver to invoke OnComplete for tracked
loads that committed, and OnFailedProvisionalLoad for tracked loads that did
not commit.

This makes it more straightforward for implementers to reason about which method
to override, and encourages separation of code for the different states into
methods which makes observer code more maintainable.

This patch also refactors the various observers to implement these methods as
needed.

BUG=627585

Committed: https://crrev.com/0259d67099838b4b0f2aabcbd3108e9952d56204
Cr-Commit-Position: refs/heads/master@{#406454}
==========

[commit-bot: I haz the power, 2016-07-20 01:57:52]

Patchset 17 (id:??) landed as
https://crrev.com/0259d67099838b4b0f2aabcbd3108e9952d56204
Cr-Commit-Position: refs/heads/master@{#406454}