Page load metrics observers cleanup

Page load metrics observers cleanup

* use consistent policy for logging bytes and resource count histograms
* define a helper macro for logging bytes and resource count histograms
* only log from FlushMetrics for committed pages
* add SimulateLoadedResource test helper
* fix logic in PopulateRequiredTimingFields

BUG=690471, 690472
Review-Url: https://codereview.chromium.org/2681193003
Cr-Commit-Position: refs/heads/master@{#450109}
Committed: https://chromium.googlesource.com/chromium/src/+/5550e325a24cf8e44fa08aa4dc298051d2b5a63a

[Bryan McQuade, 2017-02-09 14:24:19]

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

[commit-bot: I haz the power, 2017-02-09 14:24:38]

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-02-09 14:33:19]

Description was changed from

==========
Page load metrics observers cleanup

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

to

==========
Page load metrics observers cleanup

* use consistent policy for logging bytes histograms
* define a helper macro for logging bytes histograms
* only log from FlushMetrics for committed pages
* add SimulateLoadedResource test helper
* fix logic in PopulateRequiredTimingFields

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

[Bryan McQuade, 2017-02-09 14:41:28]

Description was changed from

==========
Page load metrics observers cleanup

* use consistent policy for logging bytes histograms
* define a helper macro for logging bytes histograms
* only log from FlushMetrics for committed pages
* add SimulateLoadedResource test helper
* fix logic in PopulateRequiredTimingFields

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

to

==========
Page load metrics observers cleanup

* use consistent policy for logging bytes histograms
* define a helper macro for logging bytes histograms
* only log from FlushMetrics for committed pages
* add SimulateLoadedResource test helper
* fix logic in PopulateRequiredTimingFields

BUG=690471, 690472
==========

[Bryan McQuade, 2017-02-09 14:41:47]

bmcquade@chromium.org changed reviewers:
+ jkarlin@chromium.org, ryansturm@chromium.org

[Bryan McQuade, 2017-02-09 14:41:54]

PTAL, thanks!

[commit-bot: I haz the power, 2017-02-09 15:09:37]

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

[commit-bot: I haz the power, 2017-02-09 15:09:37]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-02-09 16:18:41]

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

[commit-bot: I haz the power, 2017-02-09 16:19:09]

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-02-09 16:25:19]

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

[commit-bot: I haz the power, 2017-02-09 16:25:37]

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

[jkarlin, 2017-02-09 17:10:08]

Not looking at the data proxy stuff. Rest looks good with a couple of comments.

https://codereview.chromium.org/2681193003/diff/60001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2681193003/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:678:
DCHECK_LE(cache_bytes_, total_bytes);
Given the line before, we know that these dchecks must be true, unless one of
the bytes value is negative. Do you instead want to check at the beginning of
the function that the byte values aren't negative?

https://codereview.chromium.org/2681193003/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:687:
PAGE_BYTES_HISTOGRAM(internal::kHistogramNetworkBytes, network_bytes_);
The bytes histograms should come closer to the definition of total_bytes above.

https://codereview.chromium.org/2681193003/diff/60001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
(right):

https://codereview.chromium.org/2681193003/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:63:
if (inout_timing->first_text_paint)
This is all pretty confusing. 

How about defining something like this (I forget exact usage of Optional):

base::Optional<int> OptionalMin(base::Optional<int> a, base::Optional<int> b) {
  if (a && !b) return a;
  if (b && !a) return b;
  if (!a && !b) return a;  // doesn't matter which
  return base::Optional<int>(min(a,b));
}

Then you can use it like so:

inout_timing->first_paint = OptionalMin(OptionalMin(first_test_paint,
first_image_paint), first_contentful_paint);

[Bryan McQuade, 2017-02-09 17:15:03]

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

[commit-bot: I haz the power, 2017-02-09 17:15:25]

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

[RyanSturm, 2017-02-09 17:30:51]

ryansturm@chromium.org changed reviewers:
+ jpfeiff@chromium.org

[RyanSturm, 2017-02-09 17:30:52]

+jpfeiff@

d_r_p lgtm % jpfeiff@'s input on not sending the pingback immediately in the
"OnHidden" case (i.e., tab hidden, but not app hidden); this should only change
the page size info in the pingback and delay when it is sent.

Thanks for adding PageLoadMetricsObserverTestHarness::SimulateLoadedResource.

[jpfeiff, 2017-02-09 17:47:26]

On 2017/02/09 17:30:52, Ryan Sturm wrote:
> +jpfeiff@
> 
> d_r_p lgtm % jpfeiff@'s input on not sending the pingback immediately in the
> "OnHidden" case (i.e., tab hidden, but not app hidden); this should only
change
> the page size info in the pingback and delay when it is sent.
> 
> Thanks for adding PageLoadMetricsObserverTestHarness::SimulateLoadedResource.

yeah, removing OnHidden seems fine!

[commit-bot: I haz the power, 2017-02-09 18:16:28]

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

[commit-bot: I haz the power, 2017-02-09 18:16:29]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-02-09 18:40:28]

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

[commit-bot: I haz the power, 2017-02-09 18:41: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-02-09 18:45:10]

Thanks! Addressed comments.

https://codereview.chromium.org/2681193003/diff/60001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2681193003/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:678:
DCHECK_LE(cache_bytes_, total_bytes);
On 2017/02/09 at 17:10:07, jkarlin wrote:
> Given the line before, we know that these dchecks must be true, unless one of
> the bytes value is negative. Do you instead want to check at the beginning of
> the function that the byte values aren't negative?

Good idea, done, thanks!

https://codereview.chromium.org/2681193003/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:687:
PAGE_BYTES_HISTOGRAM(internal::kHistogramNetworkBytes, network_bytes_);
On 2017/02/09 at 17:10:07, jkarlin wrote:
> The bytes histograms should come closer to the definition of total_bytes
above.

Done

https://codereview.chromium.org/2681193003/diff/60001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
(right):

https://codereview.chromium.org/2681193003/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:63:
if (inout_timing->first_text_paint)
On 2017/02/09 at 17:10:07, jkarlin wrote:
> This is all pretty confusing. 
> 
> How about defining something like this (I forget exact usage of Optional):
> 
> base::Optional<int> OptionalMin(base::Optional<int> a, base::Optional<int> b)
{
>   if (a && !b) return a;
>   if (b && !a) return b;
>   if (!a && !b) return a;  // doesn't matter which
>   return base::Optional<int>(min(a,b));
> }
> 
> Then you can use it like so:
> 
> inout_timing->first_paint = OptionalMin(OptionalMin(first_test_paint,
first_image_paint), first_contentful_paint);

Thanks! This is much cleaner. I made this change.

[jkarlin, 2017-02-09 18:50:59]

non-d_r_p lgtm with one comment

https://codereview.chromium.org/2681193003/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
(right):

https://codereview.chromium.org/2681193003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:51:
base::Optional<base::TimeDelta> OptionalMin(
This should be in the anonymous namespace

[Bryan McQuade, 2017-02-09 18:54:59]

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

[commit-bot: I haz the power, 2017-02-09 18:55:12]

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-02-09 19:01:17]

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

[Bryan McQuade, 2017-02-09 19:01:46]

done, thanks!

https://codereview.chromium.org/2681193003/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
(right):

https://codereview.chromium.org/2681193003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:51:
base::Optional<base::TimeDelta> OptionalMin(
On 2017/02/09 at 18:50:59, jkarlin wrote:
> This should be in the anonymous namespace

ah, thanks! done.

[commit-bot: I haz the power, 2017-02-09 19:01: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, 2017-02-09 20:06:49]

bmcquade@chromium.org changed reviewers:
+ rkaplow@chromium.org
- jpfeiff@chromium.org

[Bryan McQuade, 2017-02-09 20:07:00]

rkaplow, PTAL for histograms.xml, thanks!

[commit-bot: I haz the power, 2017-02-09 20:12:07]

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

[commit-bot: I haz the power, 2017-02-09 20:12:08]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-02-09 23:26:06]

Description was changed from

==========
Page load metrics observers cleanup

* use consistent policy for logging bytes histograms
* define a helper macro for logging bytes histograms
* only log from FlushMetrics for committed pages
* add SimulateLoadedResource test helper
* fix logic in PopulateRequiredTimingFields

BUG=690471, 690472
==========

to

==========
Page load metrics observers cleanup

* use consistent policy for logging bytes and resource count histograms
* define a helper macro for logging bytes and resource count histograms
* only log from FlushMetrics for committed pages
* add SimulateLoadedResource test helper
* fix logic in PopulateRequiredTimingFields

BUG=690471, 690472
==========

[rkaplow, 2017-02-13 18:09:37]

lgtm

lg although this histograms are pretty hard to review with the level of
suffixing/prefixing!

[Bryan McQuade, 2017-02-13 18:57:14]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2017-02-13 18:57:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
ryansturm@chromium.org, jkarlin@chromium.org
Link to the patchset: https://codereview.chromium.org/2681193003/#ps140001
(title: "address comment")

[commit-bot: I haz the power, 2017-02-13 18:57:52]

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-02-13 22:05:54]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487012234696980,
"parent_rev": "6b313e9b6090d47e932bf903141d73d33bcacb3b", "commit_rev":
"5550e325a24cf8e44fa08aa4dc298051d2b5a63a"}

[commit-bot: I haz the power, 2017-02-13 22:06:32]

Description was changed from

==========
Page load metrics observers cleanup

* use consistent policy for logging bytes and resource count histograms
* define a helper macro for logging bytes and resource count histograms
* only log from FlushMetrics for committed pages
* add SimulateLoadedResource test helper
* fix logic in PopulateRequiredTimingFields

BUG=690471, 690472
==========

to

==========
Page load metrics observers cleanup

* use consistent policy for logging bytes and resource count histograms
* define a helper macro for logging bytes and resource count histograms
* only log from FlushMetrics for committed pages
* add SimulateLoadedResource test helper
* fix logic in PopulateRequiredTimingFields

BUG=690471, 690472

Review-Url: https://codereview.chromium.org/2681193003
Cr-Commit-Position: refs/heads/master@{#450109}
Committed:
https://chromium.googlesource.com/chromium/src/+/5550e325a24cf8e44fa08aa4dc29...
==========

[commit-bot: I haz the power, 2017-02-13 22:06:33]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/5550e325a24cf8e44fa08aa4dc29...