Add page/document timing UMAs for search without service worker

Add page/document timing UMAs for search without service worker

BUG=730916
Review-Url: https://codereview.chromium.org/2916033002
Cr-Commit-Position: refs/heads/master@{#478152}
Committed: https://chromium.googlesource.com/chromium/src/+/1b960fc783d7f7e632af435a141d1952c452cab1

[horo, 2017-06-01 05:38:40]

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

[commit-bot: I haz the power, 2017-06-01 05:38:57]

Dry run: CQ is trying da patch.

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

[horo, 2017-06-01 05:40:46]

horo@chromium.org changed reviewers:
+ falken@chromium.org

[horo, 2017-06-01 05:40:46]

falken@
Could you please review this?

[commit-bot: I haz the power, 2017-06-01 07:22:46]

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

[commit-bot: I haz the power, 2017-06-01 07:22:47]

Dry run: This issue passed the CQ dry run.

[falken, 2017-06-07 03:43:32]

https://codereview.chromium.org/2916033002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2916033002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:91502: <suffix name="search"
label="Custom histogram for Search"/>
for "the Google Search results page".

https://codereview.chromium.org/2916033002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2916033002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:91504: +      label="Custom histogram
for Search without service worker"/>
This will make the label something like:
"Measures the time...for main frame documents.  PageLoadMetrics from a page that
is controlled by a Service Worker; Custom histogram for Search without service
worker"

How about:
"PageLoadMetrics from a page that is controlled by a service worker, unless
otherwise specified; Custom histogram for the Google Search results page when
NOT controlled by a service worker. The ancestor histograms above "search_no_sw"
are not recorded.

[horo, 2017-06-07 06:04:13]

Thank you.

https://codereview.chromium.org/2916033002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2916033002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:91502: <suffix name="search"
label="Custom histogram for Search"/>
On 2017/06/07 03:43:32, falken wrote:
> for "the Google Search results page".

Done.

https://codereview.chromium.org/2916033002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2916033002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:91504: +      label="Custom histogram
for Search without service worker"/>
On 2017/06/07 03:43:32, falken wrote:
> This will make the label something like:
> "Measures the time...for main frame documents.  PageLoadMetrics from a page
that
> is controlled by a Service Worker; Custom histogram for Search without service
> worker"
> 
> How about:
> "PageLoadMetrics from a page that is controlled by a service worker, unless
> otherwise specified; Custom histogram for the Google Search results page when
> NOT controlled by a service worker. The ancestor histograms above
"search_no_sw"
> are not recorded.

Done.

[falken, 2017-06-07 06:07:29]

lgtm

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

https://codereview.chromium.org/2916033002/diff/10001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:91864: +             controlled by a
service worker"/>
I think it's worth adding: "The ancestor histograms above
".search_no_sw" are not recorded."

Since we've setup histograms where:
X.Y
X.Y_a
X.Y_b
X.Y_c

When X.Y_a or X.Y_b is logged, X.Y is logged too, but when X.Y_c is logged, X.Y
is not logged.

[horo, 2017-06-07 06:32:33]

Thank you.

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

https://codereview.chromium.org/2916033002/diff/10001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:91864: +             controlled by a
service worker"/>
On 2017/06/07 06:07:29, falken wrote:
> I think it's worth adding: "The ancestor histograms above
> ".search_no_sw" are not recorded."
> 
> Since we've setup histograms where:
> X.Y
> X.Y_a
> X.Y_b
> X.Y_c
> 
> When X.Y_a or X.Y_b is logged, X.Y is logged too, but when X.Y_c is logged,
X.Y
> is not logged.

Done.

[horo, 2017-06-07 06:33:39]

horo@chromium.org changed reviewers:
+ kinuko@chromium.org, mpearson@chromium.org

[horo, 2017-06-07 06:33:40]

kinuko@
Could you please review 
chrome/browser/page_load_metrics/observers/service_worker_page_load_metrics_observer*?

mpearson@
Could you please review histograms.xml?

[kinuko, 2017-06-07 07:07:26]

Hm, I see ok. lgtm

[Mark P, 2017-06-07 16:15:02]

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

https://codereview.chromium.org/2916033002/diff/30001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:91836: +             worker, unless
otherwise specified"/>
This phrasing makes me think that you're changing what you emit to this set of
histograms with the suffix "Clients.ServiceWorker".  Is that true?  If so,
please consider revising the name/suffix per best practices:
https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo...

https://codereview.chromium.org/2916033002/diff/30001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:91865: +             '.search_no_sw' are
not recorded."/>
I don't understand this last sentence.  Can you rephrase?

[horo, 2017-06-08 02:36:47]

Patchset #4 (id:50001) has been deleted

[horo, 2017-06-08 02:38:22]

Description was changed from

==========
Add page/document timing UMAs for search without service worker

BUG=727599
==========

to

==========
Add page/document timing UMAs for search without service worker

BUG=730916
==========

[horo, 2017-06-08 02:39:50]

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

[commit-bot: I haz the power, 2017-06-08 02:40:10]

Dry run: CQ is trying da patch.

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

[horo, 2017-06-08 02:40:17]

I uploaded patch set 4 which introduces "Clients.NoServiceWorker".

How about this?

[horo, 2017-06-08 04:15:20]

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

[commit-bot: I haz the power, 2017-06-08 04:15:31]

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, 2017-06-08 05:50:29]

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

[commit-bot: I haz the power, 2017-06-08 05:50:30]

Dry run: This issue passed the CQ dry run.

[Mark P, 2017-06-08 23:40:54]

histograms.xml lgtm, though you should probably have someone re-review your code
changes (lots of new histogram names, make sure they're used in the right
places, etc.)

--mark

[horo, 2017-06-08 23:42:52]

On 2017/06/08 23:40:54, Mark P wrote:
> histograms.xml lgtm, though you should probably have someone re-review your
code
> changes (lots of new histogram names, make sure they're used in the right
> places, etc.)
> 
> --mark

Thank you.

[horo, 2017-06-08 23:43:30]

falken@
Could you please review this CL again?

[falken, 2017-06-09 01:03:26]

nice. lgtm

[horo, 2017-06-09 01:08:56]

The CQ bit was checked by horo@chromium.org

[horo, 2017-06-09 01:08:57]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/2916033002/#ps90001
(title: "fix service_worker_page_load_metrics_observer.cc")

[commit-bot: I haz the power, 2017-06-09 01:09: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-06-09 01:14:17]

CQ is committing da patch.

Bot data: {"patchset_id": 90001, "attempt_start_ts": 1496970536690230,
"parent_rev": "316c57df2b949823a1fbd31d64150edb3d3af1e0", "commit_rev":
"1b960fc783d7f7e632af435a141d1952c452cab1"}

[commit-bot: I haz the power, 2017-06-09 01:14:26]

Description was changed from

==========
Add page/document timing UMAs for search without service worker

BUG=730916
==========

to

==========
Add page/document timing UMAs for search without service worker

BUG=730916

Review-Url: https://codereview.chromium.org/2916033002
Cr-Commit-Position: refs/heads/master@{#478152}
Committed:
https://chromium.googlesource.com/chromium/src/+/1b960fc783d7f7e632af435a141d...
==========

[commit-bot: I haz the power, 2017-06-09 01:14:27]

Committed patchset #5 (id:90001) as
https://chromium.googlesource.com/chromium/src/+/1b960fc783d7f7e632af435a141d...