Clean up logic to determine if a navigation is from GWS.

Clean up logic to determine if a navigation is from GWS.

This change updates the FromGWS metric logger to implement the logic
described in
https://docs.google.com/document/d/1jNPZ6Aeh0KV6umw1yZrrkfXRfxWNruwu7FELLx_cpOg/edit

BUG=604418
Committed: https://crrev.com/5ff8031245ce89165de8b803eb671b719b9657ca
Cr-Commit-Position: refs/heads/master@{#388396}

[Bryan McQuade, 2016-04-18 17:28:13]

Description was changed from

==========
Clean up logic to determine if a navigation is from GWS.

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

to

==========
Clean up logic to determine if a navigation is from GWS.

This change updates the FromGWS metric logger to implement the logic
described in
https://docs.google.com/document/d/1jNPZ6Aeh0KV6umw1yZrrkfXRfxWNruwu7FELLx_cp...

BUG=604418
==========

[Bryan McQuade, 2016-04-18 17:29:35]

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

[Bryan McQuade, 2016-04-18 17:29:44]

PTAL

[Charlie Harrison, 2016-04-18 18:24:51]

Looks good. Thanks for driving this investigation!

https://codereview.chromium.org/1891883002/diff/180001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/1891883002/diff/180001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:108:
for (size_t start_offset = 0; start_offset <= last_search_start;
Can you include a comment why you are looping here? It's not clear until the end
of the for block.

https://codereview.chromium.org/1891883002/diff/180001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/1891883002/diff/180001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc:30:
static bool IsGoogleSearchHostname(base::StringPiece host) {
Are these helpers really necessary? Can you use a method similar to an idiom in
the captcha observer?
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pag...

[Bryan McQuade, 2016-04-19 02:03:26]

Thanks! Addressed comments. PTAL.

https://codereview.chromium.org/1891883002/diff/180001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/1891883002/diff/180001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:108:
for (size_t start_offset = 0; start_offset <= last_search_start;
On 2016/04/18 at 18:24:50, csharrison wrote:
> Can you include a comment why you are looping here? It's not clear until the
end of the for block.

Good idea, done.

https://codereview.chromium.org/1891883002/diff/180001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/1891883002/diff/180001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc:30:
static bool IsGoogleSearchHostname(base::StringPiece host) {
On 2016/04/18 at 18:24:51, csharrison wrote:
> Are these helpers really necessary? Can you use a method similar to an idiom
in the captcha observer?
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pag...

Good suggestion. Switched to the data driven format.

[Bryan McQuade, 2016-04-19 02:03:27]

Thanks! Addressed comments. PTAL.

https://codereview.chromium.org/1891883002/diff/180001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/1891883002/diff/180001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:108:
for (size_t start_offset = 0; start_offset <= last_search_start;
On 2016/04/18 at 18:24:50, csharrison wrote:
> Can you include a comment why you are looping here? It's not clear until the
end of the for block.

Good idea, done.

https://codereview.chromium.org/1891883002/diff/180001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/1891883002/diff/180001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer_unittest.cc:30:
static bool IsGoogleSearchHostname(base::StringPiece host) {
On 2016/04/18 at 18:24:51, csharrison wrote:
> Are these helpers really necessary? Can you use a method similar to an idiom
in the captcha observer?
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pag...

Good suggestion. Switched to the data driven format.

[Charlie Harrison, 2016-04-19 13:09:50]

Thanks, I think data-driven works well for those particular unit tests.

lgtm but you might want to rename the histograms suffix to "Clients.FromGWS2" if
you think the change is significant enough.

[Bryan McQuade, 2016-04-19 14:40:35]

On 2016/04/19 at 13:09:50, csharrison wrote:
> Thanks, I think data-driven works well for those particular unit tests.
> 
> lgtm but you might want to rename the histograms suffix to "Clients.FromGWS2"
if you think the change is significant enough.

Good point. Agree we should rename. I updated to FromGWS2. I also refactored the
code to declare histogram names to be more consistent with our other observers,
and removed some histograms that we're migrating away from metrics for
(first_layout since first_*_paint is preferred, and dom_loading-based histograms
in favor of the new parse metric histograms).

Enough changed as a result that I think the code could use one more review.
Could you take a look when you have a chance?

[Bryan McQuade, 2016-04-19 14:54:42]

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

[Bryan McQuade, 2016-04-19 14:55:51]

+asvitkine for changes to histograms.xml

Alexei, we're changing the criteria used to log our FromGWS metrics, in
collaboration with search team. In so doing, we decided to rev the version for
these metrics, since they'll be tracking a different subset of page navigations
as a result of this change. I've updated histograms.xml to reflect this change.
PTAL, thanks!

[Alexei Svitkine (slow), 2016-04-19 15:51:56]

https://codereview.chromium.org/1891883002/diff/320001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1891883002/diff/320001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:91387: <suffix name="Clients.FromGWS"
I think you can just mark this <suffix> as obsolete if you're keeping all the
same list of affected-histograms and other suffixes.

See PrerenderSource as an example.

https://codereview.chromium.org/1891883002/diff/320001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:91390: -  <suffix
name="Clients.DocWrite.Evaluator"
Keep this here, if you're already marking this suffix group as obsolete.

[Bryan McQuade, 2016-04-19 15:59:41]

Thanks! Didn't know about being able to mark a suffix as obsolete. Updated.

https://codereview.chromium.org/1891883002/diff/320001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1891883002/diff/320001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:91387: <suffix name="Clients.FromGWS"
On 2016/04/19 at 15:51:55, Alexei Svitkine wrote:
> I think you can just mark this <suffix> as obsolete if you're keeping all the
same list of affected-histograms and other suffixes.
> 
> See PrerenderSource as an example.

Ah, thanks, didn't know about this.

I did remove a few affected-histograms that we're in the procsess of removing
generally as part of this change, such as
PageLoad.Timing2.DOMLoadingToDOMContentLoadedEventFired,
PageLoad.Timing2.DOMLoadingToFirstContentfulPaint, and
PageLoad.Timing2.NavigationToFirstLayout. These were never logged by
Clients.DocWrite.Evaluator and we want to stop logging them for
Clients.FromGWS*. I could try to mark these as deprecated explicitly in
histograms.xml if there's an easy way to do that, but I'm also fine with just
removing them as I've done here. Let me know what you prefer.

https://codereview.chromium.org/1891883002/diff/320001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:91390: -  <suffix
name="Clients.DocWrite.Evaluator"
On 2016/04/19 at 15:51:55, Alexei Svitkine wrote:
> Keep this here, if you're already marking this suffix group as obsolete.

Done

[Charlie Harrison, 2016-04-19 18:56:02]

still lgtm.

[Alexei Svitkine (slow), 2016-04-19 19:57:55]

https://codereview.chromium.org/1891883002/diff/340001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1891883002/diff/340001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:91400: -  <affected-histogram
name="PageLoad.Timing2.NavigationToFirstLayout"/>
What's the reason for removing these?

If these were logged before, we would still want these entries so that old data
with suffixes can be decoded.

[Bryan McQuade, 2016-04-19 20:04:51]

Addressed comment. PTAL. Thanks!

https://codereview.chromium.org/1891883002/diff/340001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1891883002/diff/340001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:91400: -  <affected-histogram
name="PageLoad.Timing2.NavigationToFirstLayout"/>
On 2016/04/19 at 19:57:55, Alexei Svitkine wrote:
> What's the reason for removing these?
> 
> If these were logged before, we would still want these entries so that old
data with suffixes can be decoded.

We're deprecating these, but that's really separate from this change, so I
restored them here. We'll address the deprecation in a separate change.

[Alexei Svitkine (slow), 2016-04-19 20:05:37]

lgtm

[Bryan McQuade, 2016-04-20 00:10:21]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2016-04-20 00:10:21]

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/1891883002/#ps360001
(title: " ")

[commit-bot: I haz the power, 2016-04-20 00:10:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1891883002/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1891883002/360001

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

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2016-04-22 19:19:39]

Description was changed from

==========
Clean up logic to determine if a navigation is from GWS.

This change updates the FromGWS metric logger to implement the logic
described in
https://docs.google.com/document/d/1jNPZ6Aeh0KV6umw1yZrrkfXRfxWNruwu7FELLx_cp...

BUG=604418
==========

to

==========
Clean up logic to determine if a navigation is from GWS.

This change updates the FromGWS metric logger to implement the logic
described in
https://docs.google.com/document/d/1jNPZ6Aeh0KV6umw1yZrrkfXRfxWNruwu7FELLx_cp...

BUG=604418

Committed: https://crrev.com/5ff8031245ce89165de8b803eb671b719b9657ca
Cr-Commit-Position: refs/heads/master@{#388396}
==========

[commit-bot: I haz the power, 2016-04-22 19:19:41]

Patchset 19 (id:??) landed as
https://crrev.com/5ff8031245ce89165de8b803eb671b719b9657ca
Cr-Commit-Position: refs/heads/master@{#388396}