Omnibox: Add logging of what type of page was displayed when it's used

Omnibox: Add logging of what type of page was displayed when it's used

For UMA opted-in users, this adds a new field to the omnibox event record that's uploaded when the user uses the omnibox.  In particular, it records a simple enum of what type of page the user was looking at, such as a generic web page or the new tab page.

This change only adds the field to the protocol buffer upload format, not the XML format.  If I decide later I want to add it to the XML format, I'll do it in another changelist.

BUG=
TEST=using VLOG(1) of what would be uploaded


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=134824

[Mark P, 2012-04-30 22:19:32]

Can both of you please take a look at this changelist?

thanks,
mark

[Ilya Sherman, 2012-04-30 22:28:11]

Metrics changes lgtm

http://codereview.chromium.org/10273019/diff/1/chrome/browser/autocomplete/au...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/10273019/diff/1/chrome/browser/autocomplete/au...
chrome/browser/autocomplete/autocomplete_edit.cc:545: LOG(INFO) << "Current URL
is valid: "
nit: DVLOG (or at least VLOG)?

[Peter Kasting, 2012-04-30 22:30:19]

Nit: Seems kind of like we should name |current_page| as
|{current,visible,}page_classification| instead.

http://codereview.chromium.org/10273019/diff/1/chrome/browser/autocomplete/au...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/10273019/diff/1/chrome/browser/autocomplete/au...
chrome/browser/autocomplete/autocomplete_edit.cc:545: LOG(INFO) << "Current URL
is valid: "
On 2012/04/30 22:28:12, Ilya Sherman wrote:
> nit: DVLOG (or at least VLOG)?

I'd probably remove this block entirely.

[Peter Kasting, 2012-04-30 22:30:33]

LGTM otherwise

[Mark P, 2012-05-01 00:02:52]

> Nit: Seems kind of like we should name |current_page| as
> |{current,visible,}page_classification| instead.

This is what I prefer as well, but Jim always complains that my variables names
are too long and that I too often put the type in the variable time.  We'll see
what he has to say this time.

--mark

http://codereview.chromium.org/10273019/diff/1/chrome/browser/autocomplete/au...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/10273019/diff/1/chrome/browser/autocomplete/au...
chrome/browser/autocomplete/autocomplete_edit.cc:545: LOG(INFO) << "Current URL
is valid: "
On 2012/04/30 22:30:19, Peter Kasting wrote:
> On 2012/04/30 22:28:12, Ilya Sherman wrote:
> > nit: DVLOG (or at least VLOG)?
> 
> I'd probably remove this block entirely.

Yep, not intended to be submitted.  Sorry I forgot to remove it.  Removed now.

[Peter Kasting, 2012-05-01 00:04:12]

On 2012/05/01 00:02:52, Mark P wrote:
> > Nit: Seems kind of like we should name |current_page| as
> > |{current,visible,}page_classification| instead.
> 
> This is what I prefer as well, but Jim always complains that my variables
names
> are too long and that I too often put the type in the variable time.

Those are not things to complain about.  Those are good coding practices.

Make this code the way you want it.

[Mark P, 2012-05-01 20:27:33]

Renamed current_page to current_page_classification.

I'll assume Ilya's LGTM from the metrics side (and that's he's a substitute for
jar@) and Peter's LGTM from the autocomplete side are both still valid unless I
hear otherwise within the next day.

--mark


On 2012/05/01 00:04:12, Peter Kasting wrote:
> On 2012/05/01 00:02:52, Mark P wrote:
> > > Nit: Seems kind of like we should name |current_page| as
> > > |{current,visible,}page_classification| instead.
> > 
> > This is what I prefer as well, but Jim always complains that my variables
> names
> > are too long and that I too often put the type in the variable time.
> 
> Those are not things to complain about.  Those are good coding practices.
> 
> Make this code the way you want it.

[jar (doing other things), 2012-05-01 22:00:02]

lgtm

http://codereview.chromium.org/10273019/diff/8001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/10273019/diff/8001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:1151:
OmniboxEventProto_PageClassification_INVALID_SPEC;
nit: Probably no need to wrap that line.

[Mark P, 2012-05-01 22:07:43]

Submitting in a few moments.

http://codereview.chromium.org/10273019/diff/8001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/10273019/diff/8001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:1151:
OmniboxEventProto_PageClassification_INVALID_SPEC;
On 2012/05/01 22:00:02, jar wrote:
> nit: Probably no need to wrap that line.

Done.

[commit-bot: I haz the power, 2012-05-01 22:10:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/10273019/10003

[commit-bot: I haz the power, 2012-05-01 23:54:54]

Change committed as 134824