Log absolute foreground duration for page loads before and after paint in from gws observer and cor…

Log absolute foreground duration for page loads before and after paint in from gws observer and core observer.

BUG=710683
Review-Url: https://codereview.chromium.org/2829363002
Cr-Commit-Position: refs/heads/master@{#466812}
Committed: https://chromium.googlesource.com/chromium/src/+/d8f66d80291414f24a13bc43aa24f9dbcb914e64

[mushan, 2017-04-21 23:27:07]

mushan@chromium.org changed reviewers:
+ asvitkine@chromium.org, bmcquade@chromium.org, csharrison@chromium.org

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

Thanks! Glad to have this change. Just a couple small naming requests.

https://codereview.chromium.org/2829363002/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2829363002/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:165:
"PageLoad.PageTiming.ForegroundDuration.AfterPaint2";
since this is measuring a different duration i'd like to use a name that's a bit
more different from the earlier AfterPaint. The goal with the earlier name was
to convey that it was the duration of time the page load was in the foreground
after painting happened. Here, we're measuring the full foreground duration, for
pages that painted. Can we call this
PageLoad.PageTiming.ForegroundDuration.WithPaint?

https://codereview.chromium.org/2829363002/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:167:
"PageLoad.PageTiming.ForegroundDuration.BeforePaint";
similarly, since this is for pages that didn't paint, let's use 'WithoutPaint'
or 'NoPaint' here.

[mushan, 2017-04-24 19:38:32]

On 2017/04/24 at 14:02:19, bmcquade wrote:
> Thanks! Glad to have this change. Just a couple small naming requests.
> 
>
https://codereview.chromium.org/2829363002/diff/1/chrome/browser/page_load_me...
> File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(right):
> 
>
https://codereview.chromium.org/2829363002/diff/1/chrome/browser/page_load_me...
>
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:165:
"PageLoad.PageTiming.ForegroundDuration.AfterPaint2";
> since this is measuring a different duration i'd like to use a name that's a
bit more different from the earlier AfterPaint. The goal with the earlier name
was to convey that it was the duration of time the page load was in the
foreground after painting happened. Here, we're measuring the full foreground
duration, for pages that painted. Can we call this
PageLoad.PageTiming.ForegroundDuration.WithPaint?
> 
>
https://codereview.chromium.org/2829363002/diff/1/chrome/browser/page_load_me...
>
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:167:
"PageLoad.PageTiming.ForegroundDuration.BeforePaint";
> similarly, since this is for pages that didn't paint, let's use 'WithoutPaint'
or 'NoPaint' here.

Thanks! Nice names! Updated.

[mushan, 2017-04-24 19:47:23]

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

[commit-bot: I haz the power, 2017-04-24 19:48:13]

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-04-24 19:57:00]

LGTM, thanks!

[commit-bot: I haz the power, 2017-04-24 21:02:59]

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

[commit-bot: I haz the power, 2017-04-24 21:03:00]

Dry run: This issue passed the CQ dry run.

[mushan, 2017-04-24 21:04:43]

The CQ bit was checked by mushan@chromium.org

[commit-bot: I haz the power, 2017-04-24 21:05:24]

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-04-24 21:14:35]

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

[commit-bot: I haz the power, 2017-04-24 21:14:36]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Bryan McQuade, 2017-04-24 21:15:46]

On 2017/04/24 at 21:14:36, commit-bot wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Ah, you will need asvitkine's LGTM as well in order to land this.

[mushan, 2017-04-24 21:16:27]

On 2017/04/24 at 21:15:46, bmcquade wrote:
> On 2017/04/24 at 21:14:36, commit-bot wrote:
> > Try jobs failed on following builders:
> >   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> Ah, you will need asvitkine's LGTM as well in order to land this.

Ah I forgot that. Thanks! Will wait for the approval.

[Alexei Svitkine (slow), 2017-04-24 21:21:19]

https://codereview.chromium.org/2829363002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2829363002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:124927: +             paint."/>
You can list multiple suffix labels in the same histogram_suffixes block. So
please combine the blocks.

[mushan, 2017-04-24 21:30:06]

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

[commit-bot: I haz the power, 2017-04-24 21:30:34]

Dry run: CQ is trying da patch.

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

[mushan, 2017-04-24 21:33:47]

On 2017/04/24 at 21:21:19, asvitkine wrote:
>
https://codereview.chromium.org/2829363002/diff/40001/tools/metrics/histogram...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/2829363002/diff/40001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:124927: +             paint."/>
> You can list multiple suffix labels in the same histogram_suffixes block. So
please combine the blocks.

Merged. Thanks.

[Alexei Svitkine (slow), 2017-04-24 21:49:42]

lgtm

[commit-bot: I haz the power, 2017-04-24 23:01:15]

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

[commit-bot: I haz the power, 2017-04-24 23:01:16]

Dry run: This issue passed the CQ dry run.

[mushan, 2017-04-24 23:01:39]

The CQ bit was checked by mushan@chromium.org

[mushan, 2017-04-24 23:01:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmcquade@chromium.org
Link to the patchset: https://codereview.chromium.org/2829363002/#ps60001
(title: "Merged histogram_suffixes.")

[commit-bot: I haz the power, 2017-04-24 23:02:45]

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-04-24 23:09:01]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493074899327800,
"parent_rev": "06dfd71d425ac5b0c7cef3060aee354946dc13b9", "commit_rev":
"d8f66d80291414f24a13bc43aa24f9dbcb914e64"}

[commit-bot: I haz the power, 2017-04-24 23:09:10]

Description was changed from

==========
Log absolute foreground duration for page loads before and after paint in from
gws observer and core observer.

BUG=710683
==========

to

==========
Log absolute foreground duration for page loads before and after paint in from
gws observer and core observer.

BUG=710683

Review-Url: https://codereview.chromium.org/2829363002
Cr-Commit-Position: refs/heads/master@{#466812}
Committed:
https://chromium.googlesource.com/chromium/src/+/d8f66d80291414f24a13bc43aa24...
==========

[commit-bot: I haz the power, 2017-04-24 23:09:11]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/d8f66d80291414f24a13bc43aa24...