[InstantExtended] Fixing how PageLoadSRP is emitted. Previously it was emitted only for Instant sea…

[InstantExtended] Fixing how PageLoadSRP is emitted. Previously it was emitted only for Instant search results pages. Now it should be emitted for any search results page.

These fixes include:
- Pull out checking for search results page to its own function, IsSearchResultsPageFromDefaultSearchProvider(url) on the TemplateURLService
- Add IsSearchResultsPage() to OmniboxCurrentPageDelegate to make checking within omnibox code easier
- Add a test for TempalteURL::IsSearchURL which is the heart of the above functions
- Move checking for search results pages from metrics_service.cc to uma_browsing_activity_observer.cc which is a more logical location.
- Listen to notification for NAV_ENTRY_COMMITTED rather than LOAD_STARTED to capture more events. This includes adding a control RecordAction for NAV_ENTRY_COMMITTED so we can differentiate SRP from non-SRP.

Incorporating some of the changes from https://codereview.chromium.org/20043003/ which utilize this functionality. These changes include:
- Add a page classification category for SEARCH_RESULT_PAGE_NO_SEARCH_TERM_REPLACEMENT
- Update the ClassifyPage function using the changes above to determine if search replacement is being done.
- Update the omnibox_event.proto to account for the additional category as well.

Also updates the name of the metrics since the definitions have changed.


BUG=263644, 264067
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216469

[rpetterson, 2013-07-31 22:49:21]

https://codereview.chromium.org/21395002/diff/20001/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/21395002/diff/20001/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_service.cc:1526:
web_contents->GetLastCommittedURL());
My biggest dilemma is here. I have available the LastCommittedURL and the
VisibleURL. Based on the comments in web_contents.h, I think LastCommittedURL is
the desired one. However, this results in the following behavior:
Visit www.google.com and entry a search query. Metric does not show up until the
SRP becomes the last committed url which is typically when the user navigates
away. I see the potential to lose metrics here. I also worry about the
correlation between PageLoad and this metric.

If I use the GetVisibleURL we get extra hits as pages are visible, but not
necessarily committed for loading.

Thoughts and suggestions are appreciated.

[Mark P, 2013-07-31 23:16:49]

https://codereview.chromium.org/21395002/diff/20001/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/21395002/diff/20001/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_service.cc:1526:
web_contents->GetLastCommittedURL());
On 2013/07/31 22:49:21, rpetterson wrote:
> If I use the GetVisibleURL we get extra hits as pages are visible, but not
> necessarily committed for loading.

What do you mean by this?

I know we emit PageLoads a bit more often than makes sense to me; that's not a
problem we need to tackle now.  Does using GetVisibleURL mean we're
over-aggressive at marking PageLoads as a search results page?

https://codereview.chromium.org/21395002/diff/20001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service.h (right):

https://codereview.chromium.org/21395002/diff/20001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.h:233: bool
IsSearchResultsPageFromDefaultSearchProvider(const GURL current_url);
Please consider adding a test for this.

[Peter Kasting, 2013-07-31 23:56:51]

https://codereview.chromium.org/21395002/diff/20001/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/21395002/diff/20001/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_service.cc:1526:
web_contents->GetLastCommittedURL());
On 2013/07/31 22:49:21, rpetterson wrote:
> My biggest dilemma is here. I have available the LastCommittedURL and the
> VisibleURL. Based on the comments in web_contents.h, I think LastCommittedURL
is
> the desired one. However, this results in the following behavior:
> Visit http://www.google.com and entry a search query. Metric does not show up
until the
> SRP becomes the last committed url which is typically when the user navigates
> away.

What you want depends on what this metric means and where it gets called from.

Loads go through a couple of stages.  First a load is made pending.  This means
the load has been requested, and we've sent a request to the server, but we
haven't heard back.  Then a load is committed, which means the server has
replied and we've begun transferring the page.  Finally the load is completed. 
At any stage in this process, a load can be cancelled.

So the question is, what is "LogLoadStarted" supposed to mean?  Is it supposed
to mean "log that the user requested that some load happen"?  In that case, you
want to be called as soon as an entry becomes pending, and you want the pending
entry.  Or does it mean "log that a load began to actually happen", in which
case you probably want to be called when the load commits, and get the committed
entry?

From your description above, it sounds as if you're getting called when a load
is made pending, which would explain why, when you look at the committed entry,
you don't see what you expected until the load after this one (e.g. when the
user navigates away from the page, and thus the navigation away becomes
pending).

I can't tell you what the metric is supposed to mean, so I can't tell you the
right way to go.

https://codereview.chromium.org/21395002/diff/20001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/21395002/diff/20001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:686: if (default_provider
&& default_provider->IsSearchURL(current_url))
Nit: Simpler:

  return default_provider && default_provider->IsSearchURL(current_url);

https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_current_page_delegate.h (right):

https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_current_page_delegate.h:39: virtual bool
IsSearchResultsPage() const = 0;
Why do you need this?  Nothing in the patch calls it.

https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_current_page_delegate_impl.cc (right):

https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_current_page_delegate_impl.cc:51:
web_contents->GetLastCommittedURL());
It seems wrong that e.g. OmniboxCurrentPageDelegateImpl::GetURL() maps to the
visible URL, but this function uses the committed URL.

https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:720: bool is_search_results =
Nit: Not the best name; instead of trying to think of a good one, just inline
this into the conditional.

https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:723: if (is_search_results)
Nit: While here: {}

[rpetterson, 2013-08-01 01:25:09]

Working on a test for this and will respond to comments when I upload it.

[Mark P, 2013-08-01 15:51:33]

By the way, I think Peter is right:
* for ClassifyPage, we need to use the currently visible page URL.
* for PageLoad user action, we need to use the right URL.  This depends on how
far along the URL is in the fetching/committing process, which I think requires
tracing back to when that LogLoadStarted() gets called.  If it's triggered by an
observer and you don't know how far along the URL will be in the loading process
by the time this thread hears the notification, you might be to pass along
additional information (i.e., the URL) in the notification that observers listen
for.

--mark

[rpetterson, 2013-08-01 18:05:33]

On 2013/08/01 15:51:33, Mark P wrote:
> By the way, I think Peter is right:
> * for ClassifyPage, we need to use the currently visible page URL.
> * for PageLoad user action, we need to use the right URL.  This depends on how
> far along the URL is in the fetching/committing process, which I think
requires
> tracing back to when that LogLoadStarted() gets called.  If it's triggered by
an
> observer and you don't know how far along the URL will be in the loading
process
> by the time this thread hears the notification, you might be to pass along
> additional information (i.e., the URL) in the notification that observers
listen
> for.
> 
> --mark

Agreed. I also think ClassifyPage should use VisibleURL I'm trying to determine
which is the right URL for PageLoad.

I've also found another test case which is having unexpected results. Hopefully
the test will help clean that up as well.

[rpetterson, 2013-08-05 19:46:12]

- It looks like LogLoadStarted is not the best place to be logging this
information on types of page loads because PageLoads don't necessarily get
invoked in several key cases. After talking with Samarth, I have moved it to be
triggered by a NAV_ENTRY_COMMITTED notification in
uma_browsing_activity_observer.cc. This location seems better than the
metrics_service.cc from a logical stand point as well. I recall there was
originally some objection about adding such specific metrics to
metrics_service.cc but at the time I wasn't sure of a better location.

- I've added a unittest for IsSearchResults. I've also tested this manually,
including the test case you gave in the bug.

- I've updated the proto files to incorporate Mark's CL for issue 264067. Mark,
please let me know if you'd prefer to commit those changes separately. I used
your code for some testing which is why it was included.

- Regarding the discussion on Visible versus Committed URL. I've used the
VisibleURL for the OmniboxCurrentPageDelegateImpl::IsSearchResultsPage() and the
ActiveURL from the NavigationEntry for the NavEntryCommitted action.

Mark, please let me know if there is anything special I need to do to deprecate
PageLoadSRP since I'm now using NavEntryCommitted as that more accurately
reflects what is being recorded.

https://codereview.chromium.org/21395002/diff/20001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/21395002/diff/20001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:686: if (default_provider
&& default_provider->IsSearchURL(current_url))
On 2013/07/31 23:56:51, Peter Kasting wrote:
> Nit: Simpler:
> 
>   return default_provider && default_provider->IsSearchURL(current_url);

Done.

https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_current_page_delegate.h (right):

https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_current_page_delegate.h:39: virtual bool
IsSearchResultsPage() const = 0;
On 2013/07/31 23:56:51, Peter Kasting wrote:
> Why do you need this?  Nothing in the patch calls it.

This is a partial merge from Mark's CL since the implementation of this function
uses the new one I've added. I'll pull in the rest of his CL.

https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_current_page_delegate_impl.cc (right):

https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_current_page_delegate_impl.cc:51:
web_contents->GetLastCommittedURL());
On 2013/07/31 23:56:51, Peter Kasting wrote:
> It seems wrong that e.g. OmniboxCurrentPageDelegateImpl::GetURL() maps to the
> visible URL, but this function uses the committed URL.

Agreed. They should use the same URL. Done.

https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:720: bool is_search_results =
On 2013/07/31 23:56:51, Peter Kasting wrote:
> Nit: Not the best name; instead of trying to think of a good one, just inline
> this into the conditional.

Done.

https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:723: if (is_search_results)
On 2013/07/31 23:56:51, Peter Kasting wrote:
> Nit: While here: {}

Done.

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1272: return
AutocompleteInput::SEARCH_RESULT_PAGE_NOT_DOING_SEARCH_TERM_REPLACEMENT;
Putting AutocompleteInput::SEARCH_RESULT_PAGE_NOT_DOING_SEARCH_TERM_REPLACEMENT;
on the next line is still over 80 characters.

[Mark P, 2013-08-05 21:43:19]

On Mon, Aug 5, 2013 at 12:46 PM, <rlp@chromium.org> wrote:

> - It looks like LogLoadStarted is not the best place to be logging this
> information on types of page loads because PageLoads don't necessarily get
> invoked in several key cases. After talking with Samarth, I have moved it
> to be
> triggered by a NAV_ENTRY_COMMITTED notification in
> uma_browsing_activity_**observer.cc. This location seems better than the
> metrics_service.cc from a logical stand point as well. I recall there was
> originally some objection about adding such specific metrics to
> metrics_service.cc but at the time I wasn't sure of a better location.
>

This is problematic.  We specifically put the PageLoadSRP event to be
triggered at the same time as PageLoad so we can tell which PageLoad is
which.  I think if we're putting the emit in a different place, we need
another user action go near it so we can tell when which PageLoads are SRPs
and which are not.

What are your thoughts?

--mark



>
> - I've added a unittest for IsSearchResults. I've also tested this
> manually,
> including the test case you gave in the bug.
>
> - I've updated the proto files to incorporate Mark's CL for issue 264067.
> Mark,
> please let me know if you'd prefer to commit those changes separately. I
> used
> your code for some testing which is why it was included.
>
> - Regarding the discussion on Visible versus Committed URL. I've used the
> VisibleURL for the OmniboxCurrentPageDelegateImpl**::IsSearchResultsPage()
> and the
> ActiveURL from the NavigationEntry for the NavEntryCommitted action.
>
> Mark, please let me know if there is anything special I need to do to
> deprecate
> PageLoadSRP since I'm now using NavEntryCommitted as that more accurately
> reflects what is being recorded.
>
>
>
> https://codereview.chromium.**org/21395002/diff/20001/**
>
chrome/browser/search_engines/**template_url_service.cc<https://codereview.chromium.org/21395002/diff/20001/chrome/browser/search_engines/template_url_service.cc>
> File chrome/browser/search_engines/**template_url_service.cc (right):
>
> https://codereview.chromium.**org/21395002/diff/20001/**
>
chrome/browser/search_engines/**template_url_service.cc#**newcode686<https://codereview.chromium.org/21395002/diff/20001/chrome/browser/search_engines/template_url_service.cc#newcode686>
> chrome/browser/search_engines/**template_url_service.cc:686: if
> (default_provider && default_provider->IsSearchURL(**current_url))
> On 2013/07/31 23:56:51, Peter Kasting wrote:
>
>> Nit: Simpler:
>>
>
>     return default_provider &&
>>
> default_provider->IsSearchURL(**current_url);
>
> Done.
>
>
> https://codereview.chromium.**org/21395002/diff/20001/**
>
chrome/browser/ui/omnibox/**omnibox_current_page_delegate.**h<https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox/omnibox_current_page_delegate.h>
> File chrome/browser/ui/omnibox/**omnibox_current_page_delegate.**h
> (right):
>
> https://codereview.chromium.**org/21395002/diff/20001/**
>
chrome/browser/ui/omnibox/**omnibox_current_page_delegate.**h#newcode39<https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox/omnibox_current_page_delegate.h#newcode39>
> chrome/browser/ui/omnibox/**omnibox_current_page_delegate.**h:39: virtual
> bool IsSearchResultsPage() const = 0;
> On 2013/07/31 23:56:51, Peter Kasting wrote:
>
>> Why do you need this?  Nothing in the patch calls it.
>>
>
> This is a partial merge from Mark's CL since the implementation of this
> function uses the new one I've added. I'll pull in the rest of his CL.
>
>
> https://codereview.chromium.**org/21395002/diff/20001/**
>
chrome/browser/ui/omnibox/**omnibox_current_page_delegate_**impl.cc<https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox/omnibox_current_page_delegate_impl.cc>
> File chrome/browser/ui/omnibox/**omnibox_current_page_delegate_**impl.cc
> (right):
>
> https://codereview.chromium.**org/21395002/diff/20001/**
> chrome/browser/ui/omnibox/**omnibox_current_page_delegate_**
>
impl.cc#newcode51<https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox/omnibox_current_page_delegate_impl.cc#newcode51>
> chrome/browser/ui/omnibox/**omnibox_current_page_delegate_**impl.cc:51:
> web_contents->**GetLastCommittedURL());
> On 2013/07/31 23:56:51, Peter Kasting wrote:
>
>> It seems wrong that e.g. OmniboxCurrentPageDelegateImpl**::GetURL() maps
>>
> to the
>
>> visible URL, but this function uses the committed URL.
>>
>
> Agreed. They should use the same URL. Done.
>
>
> https://codereview.chromium.**org/21395002/diff/20001/**
>
chrome/browser/ui/omnibox/**omnibox_edit_model.cc<https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox/omnibox_edit_model.cc>
> File chrome/browser/ui/omnibox/**omnibox_edit_model.cc (right):
>
> https://codereview.chromium.**org/21395002/diff/20001/**
>
chrome/browser/ui/omnibox/**omnibox_edit_model.cc#**newcode720<https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox/omnibox_edit_model.cc#newcode720>
> chrome/browser/ui/omnibox/**omnibox_edit_model.cc:720: bool
> is_search_results =
> On 2013/07/31 23:56:51, Peter Kasting wrote:
>
>> Nit: Not the best name; instead of trying to think of a good one, just
>>
> inline
>
>> this into the conditional.
>>
>
> Done.
>
>
> https://codereview.chromium.**org/21395002/diff/20001/**
>
chrome/browser/ui/omnibox/**omnibox_edit_model.cc#**newcode723<https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox/omnibox_edit_model.cc#newcode723>
> chrome/browser/ui/omnibox/**omnibox_edit_model.cc:723: if
> (is_search_results)
> On 2013/07/31 23:56:51, Peter Kasting wrote:
>
>> Nit: While here: {}
>>
>
> Done.
>
> https://codereview.chromium.**org/21395002/diff/41001/**
>
chrome/browser/ui/omnibox/**omnibox_edit_model.cc<https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/omnibox/omnibox_edit_model.cc>
> File chrome/browser/ui/omnibox/**omnibox_edit_model.cc (right):
>
> https://codereview.chromium.**org/21395002/diff/41001/**
>
chrome/browser/ui/omnibox/**omnibox_edit_model.cc#**newcode1272<https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/omnibox/omnibox_edit_model.cc#newcode1272>
> chrome/browser/ui/omnibox/**omnibox_edit_model.cc:1272: return
> AutocompleteInput::SEARCH_**RESULT_PAGE_NOT_DOING_SEARCH_**
> TERM_REPLACEMENT;
> Putting
> AutocompleteInput::SEARCH_**RESULT_PAGE_NOT_DOING_SEARCH_**
> TERM_REPLACEMENT;
> on the next line is still over 80 characters.
>
>
https://codereview.chromium.**org/21395002/<https://codereview.chromium.org/2...
>

[rlp, 2013-08-05 23:02:53]

That's what the control NavEntryCommitted is for on line 61 of
uma_browsing_activity_observer.cc.

In talking with Peter, I also need to account for instant nav entries and
will upload that soon. The code should be similar and will be in
uma_browsing_activity_observer.cc.
On Aug 5, 2013 2:43 PM, "Mark Pearson" <mpearson@chromium.org> wrote:

>
> On Mon, Aug 5, 2013 at 12:46 PM, <rlp@chromium.org> wrote:
>
>> - It looks like LogLoadStarted is not the best place to be logging this
>> information on types of page loads because PageLoads don't necessarily get
>> invoked in several key cases. After talking with Samarth, I have moved it
>> to be
>> triggered by a NAV_ENTRY_COMMITTED notification in
>> uma_browsing_activity_**observer.cc. This location seems better than the
>> metrics_service.cc from a logical stand point as well. I recall there was
>> originally some objection about adding such specific metrics to
>> metrics_service.cc but at the time I wasn't sure of a better location.
>>
>
> This is problematic.  We specifically put the PageLoadSRP event to be
> triggered at the same time as PageLoad so we can tell which PageLoad is
> which.  I think if we're putting the emit in a different place, we need
> another user action go near it so we can tell when which PageLoads are SRPs
> and which are not.
>
> What are your thoughts?
>
> --mark
>
>
>
>>
>> - I've added a unittest for IsSearchResults. I've also tested this
>> manually,
>> including the test case you gave in the bug.
>>
>> - I've updated the proto files to incorporate Mark's CL for issue 264067.
>> Mark,
>> please let me know if you'd prefer to commit those changes separately. I
>> used
>> your code for some testing which is why it was included.
>>
>> - Regarding the discussion on Visible versus Committed URL. I've used the
>> VisibleURL for the OmniboxCurrentPageDelegateImpl**::IsSearchResultsPage()
>> and the
>> ActiveURL from the NavigationEntry for the NavEntryCommitted action.
>>
>> Mark, please let me know if there is anything special I need to do to
>> deprecate
>> PageLoadSRP since I'm now using NavEntryCommitted as that more accurately
>> reflects what is being recorded.
>>
>>
>>
>> https://codereview.chromium.**org/21395002/diff/20001/**
>>
chrome/browser/search_engines/**template_url_service.cc<https://codereview.chromium.org/21395002/diff/20001/chrome/browser/search_engines/template_url_service.cc>
>> File chrome/browser/search_engines/**template_url_service.cc (right):
>>
>> https://codereview.chromium.**org/21395002/diff/20001/**
>>
chrome/browser/search_engines/**template_url_service.cc#**newcode686<https://codereview.chromium.org/21395002/diff/20001/chrome/browser/search_engines/template_url_service.cc#newcode686>
>> chrome/browser/search_engines/**template_url_service.cc:686: if
>> (default_provider && default_provider->IsSearchURL(**current_url))
>> On 2013/07/31 23:56:51, Peter Kasting wrote:
>>
>>> Nit: Simpler:
>>>
>>
>>     return default_provider &&
>>>
>> default_provider->IsSearchURL(**current_url);
>>
>> Done.
>>
>>
>> https://codereview.chromium.**org/21395002/diff/20001/**
>>
chrome/browser/ui/omnibox/**omnibox_current_page_delegate.**h<https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox/omnibox_current_page_delegate.h>
>> File chrome/browser/ui/omnibox/**omnibox_current_page_delegate.**h
>> (right):
>>
>> https://codereview.chromium.**org/21395002/diff/20001/**
>>
chrome/browser/ui/omnibox/**omnibox_current_page_delegate.**h#newcode39<https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox/omnibox_current_page_delegate.h#newcode39>
>> chrome/browser/ui/omnibox/**omnibox_current_page_delegate.**h:39: virtual
>> bool IsSearchResultsPage() const = 0;
>> On 2013/07/31 23:56:51, Peter Kasting wrote:
>>
>>> Why do you need this?  Nothing in the patch calls it.
>>>
>>
>> This is a partial merge from Mark's CL since the implementation of this
>> function uses the new one I've added. I'll pull in the rest of his CL.
>>
>>
>> https://codereview.chromium.**org/21395002/diff/20001/**
>>
chrome/browser/ui/omnibox/**omnibox_current_page_delegate_**impl.cc<https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox/omnibox_current_page_delegate_impl.cc>
>> File chrome/browser/ui/omnibox/**omnibox_current_page_delegate_**impl.cc
>> (right):
>>
>> https://codereview.chromium.**org/21395002/diff/20001/**
>> chrome/browser/ui/omnibox/**omnibox_current_page_delegate_**
>>
impl.cc#newcode51<https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox/omnibox_current_page_delegate_impl.cc#newcode51>
>> chrome/browser/ui/omnibox/**omnibox_current_page_delegate_**impl.cc:51:
>> web_contents->**GetLastCommittedURL());
>> On 2013/07/31 23:56:51, Peter Kasting wrote:
>>
>>> It seems wrong that e.g. OmniboxCurrentPageDelegateImpl**::GetURL() maps
>>>
>> to the
>>
>>> visible URL, but this function uses the committed URL.
>>>
>>
>> Agreed. They should use the same URL. Done.
>>
>>
>> https://codereview.chromium.**org/21395002/diff/20001/**
>>
chrome/browser/ui/omnibox/**omnibox_edit_model.cc<https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox/omnibox_edit_model.cc>
>> File chrome/browser/ui/omnibox/**omnibox_edit_model.cc (right):
>>
>> https://codereview.chromium.**org/21395002/diff/20001/**
>>
chrome/browser/ui/omnibox/**omnibox_edit_model.cc#**newcode720<https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox/omnibox_edit_model.cc#newcode720>
>> chrome/browser/ui/omnibox/**omnibox_edit_model.cc:720: bool
>> is_search_results =
>> On 2013/07/31 23:56:51, Peter Kasting wrote:
>>
>>> Nit: Not the best name; instead of trying to think of a good one, just
>>>
>> inline
>>
>>> this into the conditional.
>>>
>>
>> Done.
>>
>>
>> https://codereview.chromium.**org/21395002/diff/20001/**
>>
chrome/browser/ui/omnibox/**omnibox_edit_model.cc#**newcode723<https://codereview.chromium.org/21395002/diff/20001/chrome/browser/ui/omnibox/omnibox_edit_model.cc#newcode723>
>> chrome/browser/ui/omnibox/**omnibox_edit_model.cc:723: if
>> (is_search_results)
>> On 2013/07/31 23:56:51, Peter Kasting wrote:
>>
>>> Nit: While here: {}
>>>
>>
>> Done.
>>
>> https://codereview.chromium.**org/21395002/diff/41001/**
>>
chrome/browser/ui/omnibox/**omnibox_edit_model.cc<https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/omnibox/omnibox_edit_model.cc>
>> File chrome/browser/ui/omnibox/**omnibox_edit_model.cc (right):
>>
>> https://codereview.chromium.**org/21395002/diff/41001/**
>>
chrome/browser/ui/omnibox/**omnibox_edit_model.cc#**newcode1272<https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/omnibox/omnibox_edit_model.cc#newcode1272>
>> chrome/browser/ui/omnibox/**omnibox_edit_model.cc:1272: return
>> AutocompleteInput::SEARCH_**RESULT_PAGE_NOT_DOING_SEARCH_**
>> TERM_REPLACEMENT;
>> Putting
>> AutocompleteInput::SEARCH_**RESULT_PAGE_NOT_DOING_SEARCH_**
>> TERM_REPLACEMENT;
>> on the next line is still over 80 characters.
>>
>>
https://codereview.chromium.**org/21395002/<https://codereview.chromium.org/2...
>>
>
>

[rpetterson, 2013-08-06 18:23:21]

On 2013/08/05 23:02:53, rlp wrote:
> That's what the control NavEntryCommitted is for on line 61 of
> uma_browsing_activity_observer.cc.
> 
> In talking with Peter, I also need to account for instant nav entries and
> will upload that soon. The code should be similar and will be in
> uma_browsing_activity_observer.cc.

Turns out this is no longer an issue as the notification was removed.

I await your comments.

[Mark P, 2013-08-06 19:38:35]

This looks like a reasonable approach to me.

>>>
Mark, please let me know if there is anything special I need to do to deprecate
PageLoadSRP since I'm now using NavEntryCommitted as that more accurately
reflects what is being recorded.
>>>

You don't have to do anything to deprecate it.

--mark

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.h (right):

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.h:66: // The user in a search
result page that's not doing search term
nit: horizontal spacing

ditto in proto file

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.h:67: // replacement, meaning the
URL of the page shows in the omnibox.
minor nit: consider ending the sentence with a similar structure to that above
by changing
"shows in the omnibox" to "should've appeared in the omnibox before the user
started editing it"

ditto in proto file

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.h:68:
SEARCH_RESULT_PAGE_NOT_DOING_SEARCH_TERM_REPLACEMENT = 7
Note: you will have to resolve against
https://codereview.chromium.org/20587003/
, which also adds enum entries, before submitting.

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.h:69: };
As you added an entry here, you need to add the translation of it into the enum
in
AsOmniboxEventPageClassification()
in
chrome/browser/metrics/metrics_log.cc

In fact, I'm amazed this compiles without doing that--I was told that chromium
has its compiler set to crash on switch statements that either don't handle all
inputs (e.g. enums) and lack a default case.

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_current_page_delegate.h (right):

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_current_page_delegate.h:38: // Returns true if
the committed entery a search results page.
nit: spelling and grammar.

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_current_page_delegate_impl.cc (right):

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_current_page_delegate_impl.cc:50:
IsSearchResultsPageFromDefaultSearchProvider(GetURL());
optional nit: consider web_contents->GetURL()
(otherwise what's the point of making the alias on line 46, only to use it once,
on line 48?)

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1272: return
AutocompleteInput::SEARCH_RESULT_PAGE_NOT_DOING_SEARCH_TERM_REPLACEMENT;
On 2013/08/05 19:46:12, rpetterson wrote:
> Putting
AutocompleteInput::SEARCH_RESULT_PAGE_NOT_DOING_SEARCH_TERM_REPLACEMENT;
> on the next line is still over 80 characters.

Maybe you can break after the colon?

pkasting@ would know the rules for sure.

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/uma_bro...
File chrome/browser/ui/uma_browsing_activity_observer.cc (right):

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/uma_bro...
chrome/browser/ui/uma_browsing_activity_observer.cc:58: if (controller) {
What can cause controller to be NULL here?

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/uma_bro...
chrome/browser/ui/uma_browsing_activity_observer.cc:68: if (is_search_results)
optional nit: consider inlining the alias above rather than using an extra
variable.  Or at least making the alias const.

[Peter Kasting, 2013-08-06 21:32:19]

LGTM with nits.

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.h (right):

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.h:66: // The user in a search
result page that's not doing search term
On 2013/08/06 19:38:36, Mark P wrote:
> nit: horizontal spacing
> 
> ditto in proto file

Also, grammar (in both places).  "The user in"?

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_service.cc (left):

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_service.cc:1523:
content::RecordAction(content::UserMetricsAction("PageLoadSRP"));
If you're eliminating this, mark it obsolete in histograms.xml?

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service.h (right):

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.h:233: bool
IsSearchResultsPageFromDefaultSearchProvider(const GURL current_url);
Nit: Call this |url| (and update comment above).  This API doesn't care about in
what sense its argument may be "current" to some caller.

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_unittest.cc (right):

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_unittest.cc:1183: static const struct
{
Nit: Hmm, should this really be static and not just const?

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_unittest.cc:1200:
search_provider.IsSearchURL(GURL(url_data[i].url));
Nit: I'd just inline this below and save a line, it's still clear.

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_unittest.cc:1201:
EXPECT_EQ(is_search_results_page, url_data[i].result);
Nit: (expected, actual)

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_current_page_delegate_impl.cc (right):

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_current_page_delegate_impl.cc:50:
IsSearchResultsPageFromDefaultSearchProvider(GetURL());
On 2013/08/06 19:38:36, Mark P wrote:
> optional nit: consider web_contents->GetURL()
> (otherwise what's the point of making the alias on line 46, only to use it
once,
> on line 48?)

I'd probably resolve this by just inlining the first line into the second,
eliminating the temp.  I don't think avoiding using GetURL() here buys much.

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1272: return
AutocompleteInput::SEARCH_RESULT_PAGE_NOT_DOING_SEARCH_TERM_REPLACEMENT;
On 2013/08/06 19:38:36, Mark P wrote:
> On 2013/08/05 19:46:12, rpetterson wrote:
> > Putting
> AutocompleteInput::SEARCH_RESULT_PAGE_NOT_DOING_SEARCH_TERM_REPLACEMENT;
> > on the next line is still over 80 characters.
> 
> Maybe you can break after the colon?
> 
> pkasting@ would know the rules for sure.

Yeah, break after the colon.

Better, try to come up with a shorter name for the enum value, perhaps
SEARCH_RESULT_PAGE_NO_SEARCH_TERM_REPLACEMENT (saves 7 chars, seems just as
clear to me).

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/uma_bro...
File chrome/browser/ui/uma_browsing_activity_observer.cc (right):

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/uma_bro...
chrome/browser/ui/uma_browsing_activity_observer.cc:58: if (controller) {
On 2013/08/06 19:38:36, Mark P wrote:
> What can cause controller to be NULL here?

I don't think it can be.

[rpetterson, 2013-08-06 23:04:17]

Here are fixes to the comments. I'll upload a new version after rebasing but
wanted to make it easy to see the fixes.

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.h (right):

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.h:66: // The user in a search
result page that's not doing search term
On 2013/08/06 19:38:36, Mark P wrote:
> nit: horizontal spacing
> 
> ditto in proto file

Done. In the proto file as well.

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.h:66: // The user in a search
result page that's not doing search term
On 2013/08/06 21:32:19, Peter Kasting wrote:
> On 2013/08/06 19:38:36, Mark P wrote:
> > nit: horizontal spacing
> > 
> > ditto in proto file
> 
> Also, grammar (in both places).  "The user in"?

Done.

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.h:67: // replacement, meaning the
URL of the page shows in the omnibox.
On 2013/08/06 19:38:36, Mark P wrote:
> minor nit: consider ending the sentence with a similar structure to that above
> by changing
> "shows in the omnibox" to "should've appeared in the omnibox before the user
> started editing it"
> 
> ditto in proto file

Done. In the proto file as well.

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.h:68:
SEARCH_RESULT_PAGE_NOT_DOING_SEARCH_TERM_REPLACEMENT = 7
On 2013/08/06 19:38:36, Mark P wrote:
> Note: you will have to resolve against
> https://codereview.chromium.org/20587003/
> , which also adds enum entries, before submitting.

I will do a separate rebase upload after the upload to fix comment concerns.

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.h:69: };
On 2013/08/06 19:38:36, Mark P wrote:
> As you added an entry here, you need to add the translation of it into the
enum
> in
> AsOmniboxEventPageClassification()
> in
> chrome/browser/metrics/metrics_log.cc
> 
> In fact, I'm amazed this compiles without doing that--I was told that chromium
> has its compiler set to crash on switch statements that either don't handle
all
> inputs (e.g. enums) and lack a default case.

Done.

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_service.cc (left):

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_service.cc:1523:
content::RecordAction(content::UserMetricsAction("PageLoadSRP"));
On 2013/08/06 21:32:19, Peter Kasting wrote:
> If you're eliminating this, mark it obsolete in histograms.xml?

It's an action, not a histogram. According to Mark there's nothing to be done.

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service.h (right):

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.h:233: bool
IsSearchResultsPageFromDefaultSearchProvider(const GURL current_url);
On 2013/08/06 21:32:19, Peter Kasting wrote:
> Nit: Call this |url| (and update comment above).  This API doesn't care about
in
> what sense its argument may be "current" to some caller.

Done. In .cc file as well.

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_unittest.cc (right):

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_unittest.cc:1183: static const struct
{
On 2013/08/06 21:32:19, Peter Kasting wrote:
> Nit: Hmm, should this really be static and not just const?

True. Static is probably unnecessary. I was looking at the test
GoogleBaseSuggestURL which may be overly static as well.

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_unittest.cc:1200:
search_provider.IsSearchURL(GURL(url_data[i].url));
On 2013/08/06 21:32:19, Peter Kasting wrote:
> Nit: I'd just inline this below and save a line, it's still clear.

Done.

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_unittest.cc:1201:
EXPECT_EQ(is_search_results_page, url_data[i].result);
On 2013/08/06 21:32:19, Peter Kasting wrote:
> Nit: (expected, actual)

Done.

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_current_page_delegate.h (right):

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_current_page_delegate.h:38: // Returns true if
the committed entery a search results page.
On 2013/08/06 19:38:36, Mark P wrote:
> nit: spelling and grammar.

Done.

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_current_page_delegate_impl.cc (right):

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_current_page_delegate_impl.cc:50:
IsSearchResultsPageFromDefaultSearchProvider(GetURL());
On 2013/08/06 21:32:19, Peter Kasting wrote:
> On 2013/08/06 19:38:36, Mark P wrote:
> > optional nit: consider web_contents->GetURL()
> > (otherwise what's the point of making the alias on line 46, only to use it
> once,
> > on line 48?)
> 
> I'd probably resolve this by just inlining the first line into the second,
> eliminating the temp.  I don't think avoiding using GetURL() here buys much.

I was trying to make it more readable. I've cleaned it up using Peter's approach
which reduced lines of code.

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1272: return
AutocompleteInput::SEARCH_RESULT_PAGE_NOT_DOING_SEARCH_TERM_REPLACEMENT;
On 2013/08/06 21:32:19, Peter Kasting wrote:
> On 2013/08/06 19:38:36, Mark P wrote:
> > On 2013/08/05 19:46:12, rpetterson wrote:
> > > Putting
> > AutocompleteInput::SEARCH_RESULT_PAGE_NOT_DOING_SEARCH_TERM_REPLACEMENT;
> > > on the next line is still over 80 characters.
> > 
> > Maybe you can break after the colon?
> > 
> > pkasting@ would know the rules for sure.
> 
> Yeah, break after the colon.
> 
> Better, try to come up with a shorter name for the enum value, perhaps
> SEARCH_RESULT_PAGE_NO_SEARCH_TERM_REPLACEMENT (saves 7 chars, seems just as
> clear to me).

Excellent. I've changed the name. Problem avoided, but I know what to do next
time!

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/uma_bro...
File chrome/browser/ui/uma_browsing_activity_observer.cc (right):

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/uma_bro...
chrome/browser/ui/uma_browsing_activity_observer.cc:58: if (controller) {
On 2013/08/06 21:32:19, Peter Kasting wrote:
> On 2013/08/06 19:38:36, Mark P wrote:
> > What can cause controller to be NULL here?
> 
> I don't think it can be.

I was being overcautious. Since it's the source, I agree that it can't be NULL
or else what would have sent the notification?

I've pulled out the surrounding if statement.

https://codereview.chromium.org/21395002/diff/41001/chrome/browser/ui/uma_bro...
chrome/browser/ui/uma_browsing_activity_observer.cc:68: if (is_search_results)
On 2013/08/06 19:38:36, Mark P wrote:
> optional nit: consider inlining the alias above rather than using an extra
> variable.  Or at least making the alias const.

Done.

[Mark P, 2013-08-06 23:17:47]

https://codereview.chromium.org/21395002/diff/49002/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service.h (right):

https://codereview.chromium.org/21395002/diff/49002/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.h:233: bool
IsSearchResultsPageFromDefaultSearchProvider(const GURL url);
consider const GURL&

https://codereview.chromium.org/21395002/diff/49002/chrome/browser/ui/uma_bro...
File chrome/browser/ui/uma_browsing_activity_observer.cc (right):

https://codereview.chromium.org/21395002/diff/49002/chrome/browser/ui/uma_bro...
chrome/browser/ui/uma_browsing_activity_observer.cc:63:
IsSearchResultsPageFromDefaultSearchProvider(
indent four less
ditto line below (or actually you may be able to fit line below on this line
now)

https://codereview.chromium.org/21395002/diff/49002/chrome/common/metrics/pro...
File chrome/common/metrics/proto/omnibox_event.proto (right):

https://codereview.chromium.org/21395002/diff/49002/chrome/common/metrics/pro...
chrome/common/metrics/proto/omnibox_event.proto:79: // omnibox before the user
started editing it, not the search terms.
still have horizontal spacing trouble.

[rpetterson, 2013-08-06 23:41:31]

This also includes the rebase, but turns out that
https://codereview.chromium.org/20587003/ isn't through the CQ yet so I'll need
to wait on that.

https://codereview.chromium.org/21395002/diff/49002/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service.h (right):

https://codereview.chromium.org/21395002/diff/49002/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.h:233: bool
IsSearchResultsPageFromDefaultSearchProvider(const GURL url);
On 2013/08/06 23:17:47, Mark P wrote:
> consider const GURL&

Done.

https://codereview.chromium.org/21395002/diff/49002/chrome/browser/ui/uma_bro...
File chrome/browser/ui/uma_browsing_activity_observer.cc (right):

https://codereview.chromium.org/21395002/diff/49002/chrome/browser/ui/uma_bro...
chrome/browser/ui/uma_browsing_activity_observer.cc:63:
IsSearchResultsPageFromDefaultSearchProvider(
On 2013/08/06 23:17:47, Mark P wrote:
> indent four less
> ditto line below (or actually you may be able to fit line below on this line
> now)

I don't understand your comment. "load.entry->GetURL()" won't fit on the line
above and my understanding is 4 space indent to continue a line.

I've fixed the indentation for the RecordAction line which was off.

https://codereview.chromium.org/21395002/diff/49002/chrome/common/metrics/pro...
File chrome/common/metrics/proto/omnibox_event.proto (right):

https://codereview.chromium.org/21395002/diff/49002/chrome/common/metrics/pro...
chrome/common/metrics/proto/omnibox_event.proto:79: // omnibox before the user
started editing it, not the search terms.
On 2013/08/06 23:17:47, Mark P wrote:
> still have horizontal spacing trouble.
> 

Done.

[Mark P, 2013-08-06 23:57:17]

https://codereview.chromium.org/21395002/diff/49002/chrome/browser/ui/uma_bro...
File chrome/browser/ui/uma_browsing_activity_observer.cc (right):

https://codereview.chromium.org/21395002/diff/49002/chrome/browser/ui/uma_bro...
chrome/browser/ui/uma_browsing_activity_observer.cc:63:
IsSearchResultsPageFromDefaultSearchProvider(
On 2013/08/06 23:41:31, rpetterson wrote:
> On 2013/08/06 23:17:47, Mark P wrote:
> > indent four less
> > ditto line below (or actually you may be able to fit line below on this line
> > now)
> 
> I don't understand your comment. "load.entry->GetURL()" won't fit on the line
> above and my understanding is 4 space indent to continue a line.

You indent more if you're at a deeper level than before.  In this case, your
parens for close out the Profile:: line, which means the IsSearchResults
function applies to the results from the GetForProfile call.  It should be
indented relative to that.


> I've fixed the indentation for the RecordAction line which was off.

[rpetterson, 2013-08-07 00:18:24]

https://codereview.chromium.org/21395002/diff/49002/chrome/browser/ui/uma_bro...
File chrome/browser/ui/uma_browsing_activity_observer.cc (right):

https://codereview.chromium.org/21395002/diff/49002/chrome/browser/ui/uma_bro...
chrome/browser/ui/uma_browsing_activity_observer.cc:63:
IsSearchResultsPageFromDefaultSearchProvider(
On 2013/08/06 23:57:17, Mark P wrote:
> On 2013/08/06 23:41:31, rpetterson wrote:
> > On 2013/08/06 23:17:47, Mark P wrote:
> > > indent four less
> > > ditto line below (or actually you may be able to fit line below on this
line
> > > now)
> > 
> > I don't understand your comment. "load.entry->GetURL()" won't fit on the
line
> > above and my understanding is 4 space indent to continue a line.
> 
> You indent more if you're at a deeper level than before.  In this case, your
> parens for close out the Profile:: line, which means the IsSearchResults
> function applies to the results from the GetForProfile call.  It should be
> indented relative to that.
> 
> 
> > I've fixed the indentation for the RecordAction line which was off.
> 

Done. Sadly load.entry->GetURL() still doesn't fit with the braces and brackets.

[rpetterson, 2013-08-07 17:07:47]

Now merged with https://codereview.chromium.org/20587003/

[Mark P, 2013-08-07 18:00:08]

lgtm

You will need a metrics owners review.  (I'm only an owner of histograms.)
I'm adding isherman@ for this purpose.  He'll tell you that you should get
the proto change landed on the google-internal side first.  Happily, I've
already
got this process started for us ( cr/50472218 ), though it's not yet approved.

One last request: please update the change description with what this change
actually does.  The description applies to a much-earlier iteration of this
change.

--mark

[rpetterson, 2013-08-07 18:38:49]

On 2013/08/07 18:00:08, Mark P wrote:
> lgtm
> 
> You will need a metrics owners review.  (I'm only an owner of histograms.)
> I'm adding isherman@ for this purpose.  He'll tell you that you should get
> the proto change landed on the google-internal side first.  Happily, I've
> already
> got this process started for us ( cr/50472218 ), though it's not yet approved.
> 
> One last request: please update the change description with what this change
> actually does.  The description applies to a much-earlier iteration of this
> change.
> 
> --mark

Thanks for taking care of the google-internal part!

I've updated the change description.

Is there someone other than Ilya that we can add to this change and yours? It
looks like Ilya is away until 8/25 and it'd be great to get this into M30.

[Mark P, 2013-08-07 18:40:23]

On Wed, Aug 7, 2013 at 11:38 AM, <rlp@chromium.org> wrote:

> On 2013/08/07 18:00:08, Mark P wrote:
>
>> lgtm
>>
>
>  You will need a metrics owners review.  (I'm only an owner of histograms.)
>> I'm adding isherman@ for this purpose.  He'll tell you that you should
>> get
>> the proto change landed on the google-internal side first.  Happily, I've
>> already
>> got this process started for us ( cr/50472218 ), though it's not yet
>> approved.
>>
>
>  One last request: please update the change description with what this
>> change
>> actually does.  The description applies to a much-earlier iteration of
>> this
>> change.
>>
>
>  --mark
>>
>
> Thanks for taking care of the google-internal part!
>
> I've updated the change description.
>
> Is there someone other than Ilya that we can add to this change and yours?
> It
> looks like Ilya is away


He's not away yet!  He's away starting this Friday.

--mark



> until 8/25 and it'd be great to get this into M30.
>
>
https://codereview.chromium.**org/21395002/<https://codereview.chromium.org/2...
>

[rpetterson, 2013-08-07 18:40:58]

So it looks like jar@ and isherman@ are both away for the next week or two. 

Switching to rtenneti@.

[rpetterson, 2013-08-07 18:41:36]

Ack! Misread the dates. Fixing :P

[ramant (doing other things), 2013-08-07 20:07:36]

lgtm

[Mark P, 2013-08-07 23:30:47]

The corresponding google-internal change has landed.

I think you have all the necessary approvals for this change (correct me if I'm
wrong).  Please feel free to submit as your leisure.

Oh, and thanks for doing this.

--mark

[Ilya Sherman, 2013-08-08 07:53:46]

metrics changes lgtm just in case my rubber stamp is still needed.

[commit-bot: I haz the power, 2013-08-08 19:42:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlp@chromium.org/21395002/84013

[commit-bot: I haz the power, 2013-08-08 23:00:21]

Change committed as 216469