Log NTP hovers in 1993 clients

Log NTP hovers in 1993 clients

Log the number of times a user mouses over a tile or title on the NTP, for 1993 clients.

BUG=254023
TEST=Count mouseovers on 1993 NTP tile/title elements. Go to about:histograms to verify that the correct number has been logged to "NewTabPage.NumberOfMouseOvers" histogram.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=210398

[annark1, 2013-06-21 21:41:39]

Hi Philippe,

Here's the CL for logging NTP hovers in 1993 clients. PTAL.

Thank you,
Anna

[beaudoin, 2013-06-22 01:14:52]

+mpearson who may have an opinion, as he's the one who suggested "# of hover
over tiles" as a confusion metric.

I just did a very quick review. Looks good and the logic seems to work. Don't
hesitate to ask another reviewer if you want to land this while I'm away next
week.

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/resources/l...
File chrome/browser/resources/local_ntp/most_visited_thumbnail.js (right):

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/resources/l...
chrome/browser/resources/local_ntp/most_visited_thumbnail.js:20:
link.addEventListener('click', handleClick);
I don't see handleClick defined anywhere. Also, whi do you put it only on
DomainElements?

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/resources/l...
chrome/browser/resources/local_ntp/most_visited_thumbnail.js:31: };
Would it make sense to move this to createMostVisitedLink and add the event
listeners in there? You seem to need them both for DomainElement and regular
thumbnails.

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/resources/l...
File chrome/browser/resources/local_ntp/most_visited_title.js (right):

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/resources/l...
chrome/browser/resources/local_ntp/most_visited_title.js:21: };
Seems to be duplicated code, one more reason to add all that in
createMostVisited.

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:254: class NtpLoggingUserData
Please document the class.

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:257: virtual
~NtpLoggingUserData() {}
Indentation is wrong in the class:
one space before "public:" or "private:"
two spaces before methods/members.

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:261:
ntp_logging_data_[iframe_pos] += 1;
++

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:264: // Sums the values in the
|ntp_logging_data_| map and logs to histogram.
Clarify to indicate that you are summing over every iframe.

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:274: }
Please check with asvitkine@ that you're using this correctly. In particular,
should you manually clamp result to 10000?

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:1093: }
Nit: No {}

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:1646: }
Nit: no {}

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:1955:
data->increment_iframe_hovers(pos);
Do you really have to keep the "pos" here? If you're only interested in total
number of hovers, you could keep a single int and keep a running sum in it.
There is not really a point in keeping it in an array...

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_page.h (right):

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_page.h:93: virtual void LogIframeHover(int pos)
= 0;
Please add a comment to indicate that this is used for UMA logging.

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_page.h:274: void OnSearchBoxLogIframeHover(int
page_id, int pos);
Same here, not sure you need the pos.

https://codereview.chromium.org/17526008/diff/2001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/searchbox_api.js (right):

https://codereview.chromium.org/17526008/diff/2001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/searchbox_api.js:262: 
Remove blank line.

https://codereview.chromium.org/17526008/diff/2001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/searchbox_api.js:300: };
Please run this by Jered since you're changing the searchbox API.

https://codereview.chromium.org/17526008/diff/2001/chrome/renderer/searchbox/...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/17526008/diff/2001/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox_extension.cc:1484:
SearchBox::Get(render_view)->LogIframeHover(pos);
Again, not sure you need the pos here.

[annark1, 2013-06-25 16:01:38]

PTAL

palmer@ for security review

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/resources/l...
File chrome/browser/resources/local_ntp/most_visited_thumbnail.js (right):

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/resources/l...
chrome/browser/resources/local_ntp/most_visited_thumbnail.js:20:
link.addEventListener('click', handleClick);
Should not have been there. Sorry, I didn't catch it from an early iteration.
On 2013/06/22 01:14:53, beaudoin wrote:
> I don't see handleClick defined anywhere. Also, whi do you put it only on
> DomainElements?

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/resources/l...
chrome/browser/resources/local_ntp/most_visited_thumbnail.js:31: };
On 2013/06/22 01:14:53, beaudoin wrote:
> Would it make sense to move this to createMostVisitedLink and add the event
> listeners in there? You seem to need them both for DomainElement and regular
> thumbnails.

Done.

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/resources/l...
File chrome/browser/resources/local_ntp/most_visited_title.js (right):

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/resources/l...
chrome/browser/resources/local_ntp/most_visited_title.js:21: };
On 2013/06/22 01:14:53, beaudoin wrote:
> Seems to be duplicated code, one more reason to add all that in
> createMostVisited.

Done.

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:254: class NtpLoggingUserData
On 2013/06/22 01:14:53, beaudoin wrote:
> Please document the class.

Done.

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:257: virtual
~NtpLoggingUserData() {}
On 2013/06/22 01:14:53, beaudoin wrote:
> Indentation is wrong in the class:
> one space before "public:" or "private:"
> two spaces before methods/members.

Done.

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:261:
ntp_logging_data_[iframe_pos] += 1;
On 2013/06/22 01:14:53, beaudoin wrote:
> ++

Done.

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:264: // Sums the values in the
|ntp_logging_data_| map and logs to histogram.
changed to just an int.
On 2013/06/22 01:14:53, beaudoin wrote:
> Clarify to indicate that you are summing over every iframe.

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:274: }
Alexei suggested just using the default UMA_HISTOGRAM_COUNTS macro.
On 2013/06/22 01:14:53, beaudoin wrote:
> Please check with asvitkine@ that you're using this correctly. In particular,
> should you manually clamp result to 10000?

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:1093: }
On 2013/06/22 01:14:53, beaudoin wrote:
> Nit: No {}

Done.

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:1646: }
On 2013/06/22 01:14:53, beaudoin wrote:
> Nit: no {}

Done.

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:1955:
data->increment_iframe_hovers(pos);
Yes, I agree that only an int is needed to just log total number of hovers. This
setup had stemmed from a discussion with Mathieu that further down the line we
may want to know more information on a per tile basis, hence logging by
position. However, as that isn't used/implemented in this patch, I've removed
the position variable.

On 2013/06/22 01:14:53, beaudoin wrote:
> Do you really have to keep the "pos" here? If you're only interested in total
> number of hovers, you could keep a single int and keep a running sum in it.
> There is not really a point in keeping it in an array...

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_page.h (right):

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_page.h:93: virtual void LogIframeHover(int pos)
= 0;
On 2013/06/22 01:14:53, beaudoin wrote:
> Please add a comment to indicate that this is used for UMA logging.

Done.

https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_page.h:274: void OnSearchBoxLogIframeHover(int
page_id, int pos);
On 2013/06/22 01:14:53, beaudoin wrote:
> Same here, not sure you need the pos.

Done.

https://codereview.chromium.org/17526008/diff/2001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/searchbox_api.js (right):

https://codereview.chromium.org/17526008/diff/2001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/searchbox_api.js:262: 
On 2013/06/22 01:14:53, beaudoin wrote:
> Remove blank line.

Done.

https://codereview.chromium.org/17526008/diff/2001/chrome/renderer/searchbox/...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/17526008/diff/2001/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox_extension.cc:1484:
SearchBox::Get(render_view)->LogIframeHover(pos);
On 2013/06/22 01:14:53, beaudoin wrote:
> Again, not sure you need the pos here.

Done.

[annark, 2013-06-25 16:03:46]

@asvitkine for histograms.xml
@jered for everything else

Thanks very much!
Anna


On Tue, Jun 25, 2013 at 12:01 PM, <annark@chromium.org> wrote:

> PTAL
>
> palmer@ for security review
>
>
>
>
> https://codereview.chromium.**org/17526008/diff/2001/chrome/**
>
browser/resources/local_ntp/**most_visited_thumbnail.js<https://codereview.chromium.org/17526008/diff/2001/chrome/browser/resources/local_ntp/most_visited_thumbnail.js>
> File chrome/browser/resources/**local_ntp/most_visited_**thumbnail.js
> (right):
>
> https://codereview.chromium.**org/17526008/diff/2001/chrome/**
>
browser/resources/local_ntp/**most_visited_thumbnail.js#**newcode20<https://codereview.chromium.org/17526008/diff/2001/chrome/browser/resources/local_ntp/most_visited_thumbnail.js#newcode20>
> chrome/browser/resources/**local_ntp/most_visited_**thumbnail.js:20:
> link.addEventListener('click', handleClick);
> Should not have been there. Sorry, I didn't catch it from an early
> iteration.
>
> On 2013/06/22 01:14:53, beaudoin wrote:
>
>> I don't see handleClick defined anywhere. Also, whi do you put it only
>>
> on
>
>> DomainElements?
>>
>
> https://codereview.chromium.**org/17526008/diff/2001/chrome/**
>
browser/resources/local_ntp/**most_visited_thumbnail.js#**newcode31<https://codereview.chromium.org/17526008/diff/2001/chrome/browser/resources/local_ntp/most_visited_thumbnail.js#newcode31>
> chrome/browser/resources/**local_ntp/most_visited_**thumbnail.js:31: };
> On 2013/06/22 01:14:53, beaudoin wrote:
>
>> Would it make sense to move this to createMostVisitedLink and add the
>>
> event
>
>> listeners in there? You seem to need them both for DomainElement and
>>
> regular
>
>> thumbnails.
>>
>
> Done.
>
>
> https://codereview.chromium.**org/17526008/diff/2001/chrome/**
>
browser/resources/local_ntp/**most_visited_title.js<https://codereview.chromium.org/17526008/diff/2001/chrome/browser/resources/local_ntp/most_visited_title.js>
> File chrome/browser/resources/**local_ntp/most_visited_title.**js (right):
>
> https://codereview.chromium.**org/17526008/diff/2001/chrome/**
>
browser/resources/local_ntp/**most_visited_title.js#**newcode21<https://codereview.chromium.org/17526008/diff/2001/chrome/browser/resources/local_ntp/most_visited_title.js#newcode21>
> chrome/browser/resources/**local_ntp/most_visited_title.**js:21: };
> On 2013/06/22 01:14:53, beaudoin wrote:
>
>> Seems to be duplicated code, one more reason to add all that in
>> createMostVisited.
>>
>
> Done.
>
>
> https://codereview.chromium.**org/17526008/diff/2001/chrome/**
>
browser/ui/search/instant_**controller.cc<https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/instant_controller.cc>
> File chrome/browser/ui/search/**instant_controller.cc (right):
>
> https://codereview.chromium.**org/17526008/diff/2001/chrome/**
>
browser/ui/search/instant_**controller.cc#newcode254<https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/instant_controller.cc#newcode254>
> chrome/browser/ui/search/**instant_controller.cc:254: class
> NtpLoggingUserData
> On 2013/06/22 01:14:53, beaudoin wrote:
>
>> Please document the class.
>>
>
> Done.
>
>
> https://codereview.chromium.**org/17526008/diff/2001/chrome/**
>
browser/ui/search/instant_**controller.cc#newcode257<https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/instant_controller.cc#newcode257>
> chrome/browser/ui/search/**instant_controller.cc:257: virtual
> ~NtpLoggingUserData() {}
> On 2013/06/22 01:14:53, beaudoin wrote:
>
>> Indentation is wrong in the class:
>> one space before "public:" or "private:"
>> two spaces before methods/members.
>>
>
> Done.
>
>
> https://codereview.chromium.**org/17526008/diff/2001/chrome/**
>
browser/ui/search/instant_**controller.cc#newcode261<https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/instant_controller.cc#newcode261>
> chrome/browser/ui/search/**instant_controller.cc:261:
> ntp_logging_data_[iframe_pos] += 1;
> On 2013/06/22 01:14:53, beaudoin wrote:
>
>> ++
>>
>
> Done.
>
>
> https://codereview.chromium.**org/17526008/diff/2001/chrome/**
>
browser/ui/search/instant_**controller.cc#newcode264<https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/instant_controller.cc#newcode264>
> chrome/browser/ui/search/**instant_controller.cc:264: // Sums the values
> in the |ntp_logging_data_| map and logs to histogram.
> changed to just an int.
>
> On 2013/06/22 01:14:53, beaudoin wrote:
>
>> Clarify to indicate that you are summing over every iframe.
>>
>
> https://codereview.chromium.**org/17526008/diff/2001/chrome/**
>
browser/ui/search/instant_**controller.cc#newcode274<https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/instant_controller.cc#newcode274>
> chrome/browser/ui/search/**instant_controller.cc:274: }
> Alexei suggested just using the default UMA_HISTOGRAM_COUNTS macro.
>
> On 2013/06/22 01:14:53, beaudoin wrote:
>
>> Please check with asvitkine@ that you're using this correctly. In
>>
> particular,
>
>> should you manually clamp result to 10000?
>>
>
> https://codereview.chromium.**org/17526008/diff/2001/chrome/**
>
browser/ui/search/instant_**controller.cc#newcode1093<https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/instant_controller.cc#newcode1093>
> chrome/browser/ui/search/**instant_controller.cc:1093: }
> On 2013/06/22 01:14:53, beaudoin wrote:
>
>> Nit: No {}
>>
>
> Done.
>
> https://codereview.chromium.**org/17526008/diff/2001/chrome/**
>
browser/ui/search/instant_**controller.cc#newcode1646<https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/instant_controller.cc#newcode1646>
> chrome/browser/ui/search/**instant_controller.cc:1646: }
> On 2013/06/22 01:14:53, beaudoin wrote:
>
>> Nit: no {}
>>
>
> Done.
>
>
> https://codereview.chromium.**org/17526008/diff/2001/chrome/**
>
browser/ui/search/instant_**controller.cc#newcode1955<https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/instant_controller.cc#newcode1955>
> chrome/browser/ui/search/**instant_controller.cc:1955:
> data->increment_iframe_hovers(**pos);
> Yes, I agree that only an int is needed to just log total number of
> hovers. This setup had stemmed from a discussion with Mathieu that
> further down the line we may want to know more information on a per tile
> basis, hence logging by position. However, as that isn't
> used/implemented in this patch, I've removed the position variable.
>
>
> On 2013/06/22 01:14:53, beaudoin wrote:
>
>> Do you really have to keep the "pos" here? If you're only interested
>>
> in total
>
>> number of hovers, you could keep a single int and keep a running sum
>>
> in it.
>
>> There is not really a point in keeping it in an array...
>>
>
> https://codereview.chromium.**org/17526008/diff/2001/chrome/**
>
browser/ui/search/instant_**page.h<https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/instant_page.h>
> File chrome/browser/ui/search/**instant_page.h (right):
>
> https://codereview.chromium.**org/17526008/diff/2001/chrome/**
>
browser/ui/search/instant_**page.h#newcode93<https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/instant_page.h#newcode93>
> chrome/browser/ui/search/**instant_page.h:93: virtual void
> LogIframeHover(int pos) = 0;
> On 2013/06/22 01:14:53, beaudoin wrote:
>
>> Please add a comment to indicate that this is used for UMA logging.
>>
>
> Done.
>
>
> https://codereview.chromium.**org/17526008/diff/2001/chrome/**
>
browser/ui/search/instant_**page.h#newcode274<https://codereview.chromium.org/17526008/diff/2001/chrome/browser/ui/search/instant_page.h#newcode274>
> chrome/browser/ui/search/**instant_page.h:274: void
> OnSearchBoxLogIframeHover(int page_id, int pos);
> On 2013/06/22 01:14:53, beaudoin wrote:
>
>> Same here, not sure you need the pos.
>>
>
> Done.
>
>
> https://codereview.chromium.**org/17526008/diff/2001/chrome/**
>
renderer/resources/extensions/**searchbox_api.js<https://codereview.chromium.org/17526008/diff/2001/chrome/renderer/resources/extensions/searchbox_api.js>
> File chrome/renderer/resources/**extensions/searchbox_api.js (right):
>
> https://codereview.chromium.**org/17526008/diff/2001/chrome/**
>
renderer/resources/extensions/**searchbox_api.js#newcode262<https://codereview.chromium.org/17526008/diff/2001/chrome/renderer/resources/extensions/searchbox_api.js#newcode262>
> chrome/renderer/resources/**extensions/searchbox_api.js:**262:
> On 2013/06/22 01:14:53, beaudoin wrote:
>
>> Remove blank line.
>>
>
> Done.
>
>
> https://codereview.chromium.**org/17526008/diff/2001/chrome/**
>
renderer/searchbox/searchbox_**extension.cc<https://codereview.chromium.org/17526008/diff/2001/chrome/renderer/searchbox/searchbox_extension.cc>
> File chrome/renderer/searchbox/**searchbox_extension.cc (right):
>
> https://codereview.chromium.**org/17526008/diff/2001/chrome/**
>
renderer/searchbox/searchbox_**extension.cc#newcode1484<https://codereview.chromium.org/17526008/diff/2001/chrome/renderer/searchbox/searchbox_extension.cc#newcode1484>
> chrome/renderer/searchbox/**searchbox_extension.cc:1484:
> SearchBox::Get(render_view)->**LogIframeHover(pos);
> On 2013/06/22 01:14:53, beaudoin wrote:
>
>> Again, not sure you need the pos here.
>>
>
> Done.
>
>
https://codereview.chromium.**org/17526008/<https://codereview.chromium.org/1...
>

[Alexei Svitkine (slow), 2013-06-25 16:58:35]

Could you set the CL description properly? It seems to be blank.

Please include the title of the CL as the first line.

[annark, 2013-06-25 17:11:18]

Done.


On Tue, Jun 25, 2013 at 12:58 PM, <asvitkine@chromium.org> wrote:

> Could you set the CL description properly? It seems to be blank.
>
> Please include the title of the CL as the first line.
>
>
https://codereview.chromium.**org/17526008/<https://codereview.chromium.org/1...
>

[Alexei Svitkine (slow), 2013-06-25 17:14:22]

Please also create a crbug for this and set the BUG= appropriately.

[Alexei Svitkine (slow), 2013-06-25 17:21:20]

https://codereview.chromium.org/17526008/diff/17001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/17526008/diff/17001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:282: };
Should this have DISALLOW_COPY_AND_ASSIGN(NtpLoggingUserData)?

https://codereview.chromium.org/17526008/diff/17001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/17526008/diff/17001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:1488:
UMA_HISTOGRAM_ENUMERATION(histogram_name, value, boundary);
Don't use the macro if histogram_name can be different between calls.

Instead, inline the code that the macro expands to modulo the caching.

[Mark P, 2013-06-25 19:04:32]

Alexei found one bug.

I have another concern, mentioned using the review tool below.

In general, have you tested this yourself using about:histograms?

Also, what happens if a user has multiple new tab pages open at once?  What gets
recorded?  (e.g., if I hover on one page and switch to another and use it to
navigate, what gets recorded?)

--mark

https://codereview.chromium.org/17526008/diff/17001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/17526008/diff/17001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:270:
UMA_HISTOGRAM_COUNTS("NewTabPage.NumberOfMouseOvers",
Why do you only log if the number is greater than 0?  Is it impossible to select
these without using the mouse?  (e.g., tab through that part of the page?)

[annark1, 2013-06-25 21:04:11]

Thanks Alexei and Mark. I've addressed your comments, PTAL.

https://codereview.chromium.org/17526008/diff/17001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/17526008/diff/17001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:270:
UMA_HISTOGRAM_COUNTS("NewTabPage.NumberOfMouseOvers",
As per our offline discussion, I've addressed this in the documentation both
here and in histograms.xml.
On 2013/06/25 19:04:32, Mark P wrote:
> Why do you only log if the number is greater than 0?  Is it impossible to
select
> these without using the mouse?  (e.g., tab through that part of the page?)

https://codereview.chromium.org/17526008/diff/17001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:282: };
On 2013/06/25 17:21:20, Alexei Svitkine wrote:
> Should this have DISALLOW_COPY_AND_ASSIGN(NtpLoggingUserData)?

Done.

https://codereview.chromium.org/17526008/diff/17001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/17526008/diff/17001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:1488:
UMA_HISTOGRAM_ENUMERATION(histogram_name, value, boundary);
On 2013/06/25 17:21:20, Alexei Svitkine wrote:
> Don't use the macro if histogram_name can be different between calls.
> 
> Instead, inline the code that the macro expands to modulo the caching.

Done.

[Jered, 2013-06-26 16:37:56]

https://codereview.chromium.org/17526008/diff/41001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/17526008/diff/41001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:254: // Helper class for logging
data from the NTP. Attached to each InstantNTP
instant_controller.cc feels like the wrong place for this class. Since it's
bound on NTP creation, how about instant_ntp.cc, from InitContents perhaps.

https://codereview.chromium.org/17526008/diff/41001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:256: class NtpLoggingUserData
Ntp -> NTP.

https://codereview.chromium.org/17526008/diff/41001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:269: // current tab is replaced.
Does not log 0 mouseover cases (for instance if a
Why not log the case with 0 hovers? This seems like interesting counterfactual
information. For example, I think it'd be useful to know how many interactions
had hovers at all to assess whether this is a common user action.

https://codereview.chromium.org/17526008/diff/41001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:285: int number_of_mouseovers;
Member variables should end with _.

https://codereview.chromium.org/17526008/diff/41001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1725: if
(NtpLoggingUserData::FromWebContents(browser_->GetActiveWebContents())) {
Why are you calling this here?

https://codereview.chromium.org/17526008/diff/41001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/17526008/diff/41001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:1490: if (!args[2]->IsNull() &&
args[2]->IsNumber()) {
What is up with this code? I don't see where it's invoked, so I'm not sure what
it is supposed to be doing.

[palmer, 2013-06-27 21:36:22]

IPC security LGTM

[annark1, 2013-06-28 15:30:49]

I've made some changes based on offline discussions with Mark and Jered. Namely,
now we do log 0 mouseover cases, as adding a WebContentsObserver to the logging
data class has made it  possible to do so without getting extra, erroneous
logging calls. PTAL.

Thanks,
Anna

https://codereview.chromium.org/17526008/diff/41001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/17526008/diff/41001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:254: // Helper class for logging
data from the NTP. Attached to each InstantNTP
On 2013/06/26 16:37:57, Jered wrote:
> instant_controller.cc feels like the wrong place for this class. Since it's
> bound on NTP creation, how about instant_ntp.cc, from InitContents perhaps.

Done.

https://codereview.chromium.org/17526008/diff/41001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:256: class NtpLoggingUserData
On 2013/06/26 16:37:57, Jered wrote:
> Ntp -> NTP.

Done.

https://codereview.chromium.org/17526008/diff/41001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:269: // current tab is replaced.
Does not log 0 mouseover cases (for instance if a
After offline discussions with Jered and Mark Pearson, I've now changed this so
we do log 0 mouseover cases.

On 2013/06/26 16:37:57, Jered wrote:
> Why not log the case with 0 hovers? This seems like interesting counterfactual
> information. For example, I think it'd be useful to know how many interactions
> had hovers at all to assess whether this is a common user action.

https://codereview.chromium.org/17526008/diff/41001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:285: int number_of_mouseovers;
On 2013/06/26 16:37:57, Jered wrote:
> Member variables should end with _.

Done.

https://codereview.chromium.org/17526008/diff/41001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1725: if
(NtpLoggingUserData::FromWebContents(browser_->GetActiveWebContents())) {
I was calling this here to capture the cases when the user navigates to a URL
from the NTP but it was called more often than was needed, hence why before I
was not logging 0 mouseover cases. My new patchset  adds a WebContentsObserver
to the NTPLoggingUserData class, whose function NavigationEntryCommitted is used
to trigger the log_number_of_mouseover function when the user navigates to URLs
from the NTP. So it's no longer necessary to call the function here.

On 2013/06/26 16:37:57, Jered wrote:
> Why are you calling this here?

https://codereview.chromium.org/17526008/diff/41001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/17526008/diff/41001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:1490: if (!args[2]->IsNull() &&
args[2]->IsNumber()) {
This was intended to make the LogEvent function more general, but as there are
no other features/logging events using it at this time, I've removed it.
On 2013/06/26 16:37:57, Jered wrote:
> What is up with this code? I don't see where it's invoked, so I'm not sure
what
> it is supposed to be doing.

[Jered, 2013-06-28 16:24:17]

This way seems better to me, since I think an increase in non-zero hover counts
would be a key indicator of surprise/confusion. Some comments below.

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1045: if (ntp_ &&
(chrome::MatchesOriginAndPath(
Instead, say
if (chrome::IsInstantNTP(contents))

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1049:
ntp_->LogTotalMouseovers(contents);
ntp_ is a preloaded new tab page, not the one that has just been deactivated.
This method should be static, so this call would just be
InstantNTP::LogTotalMouseovers(contents);

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1891: void
InstantController::LogIframeHover() {
Omit this method on InstantController. Handle this entirely from InstantPage.

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_ntp.cc (right):

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:46: virtual void 
NavigationEntryCommitted OVERRIDE(
Remove extra space after "void".

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:49: GURL(instant_url_),
load_details.previous_url))
Indent 4 more spaces.

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:57: instant_url_("Needs To Be Set") {}
Omit this default value.

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_ntp.h (right):

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.h:41: void
LogIframeHover(content::WebContents* contents);
Using both "IframeHover" and "mouseover" makes it seem like these are counting
two different things. Instead, how about CountMouseover() and
EmitMouseoverCount().

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_page.cc (right):

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page.cc:266: delegate_->LogIframeHover();
Here, call InstantNTP::LogIframeHover(contents()).

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_page.h (right):

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page.h:95: // Used to log in UMA the total
number of mouseovers over NTP tiles/titles.
Remove this.

[annark1, 2013-06-28 18:38:22]

Thanks for the really helpful suggestions, Jered. I have one question regarding
setting a 'dummy' variable for the instant_url_ variable (see instant_ntp.cc) to
avoid issues when the previous_url provided in NavigationEntryCommitted is
blank.

Anna

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1045: if (ntp_ &&
(chrome::MatchesOriginAndPath(
On 2013/06/28 16:24:17, Jered wrote:
> Instead, say
> if (chrome::IsInstantNTP(contents))

Done.

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1049:
ntp_->LogTotalMouseovers(contents);
On 2013/06/28 16:24:17, Jered wrote:
> ntp_ is a preloaded new tab page, not the one that has just been deactivated.
> This method should be static, so this call would just be
> InstantNTP::LogTotalMouseovers(contents);

Done.

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1891: void
InstantController::LogIframeHover() {
On 2013/06/28 16:24:17, Jered wrote:
> Omit this method on InstantController. Handle this entirely from InstantPage.

Done.

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_ntp.cc (right):

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:46: virtual void 
NavigationEntryCommitted OVERRIDE(
On 2013/06/28 16:24:17, Jered wrote:
> Remove extra space after "void".

Done.

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:49: GURL(instant_url_),
load_details.previous_url))
On 2013/06/28 16:24:17, Jered wrote:
> Indent 4 more spaces.

Done.

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:57: instant_url_("Needs To Be Set") {}
The reason this is here is because on start up the NavigationEntryCommitted
function seems to get fired twice, and in those cases the load_details'
previous_url is blank. Is there some dummy value I can put in for these types of
cases, so there's no chance of it seeming like there is a match when in fact
there isn't?

On 2013/06/28 16:24:17, Jered wrote:
> Omit this default value.

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_page.cc (right):

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page.cc:266: delegate_->LogIframeHover();
On 2013/06/28 16:24:17, Jered wrote:
> Here, call InstantNTP::LogIframeHover(contents()).

Done.

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_page.h (right):

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page.h:95: // Used to log in UMA the total
number of mouseovers over NTP tiles/titles.
On 2013/06/28 16:24:17, Jered wrote:
> Remove this.

Done.

[Jered, 2013-06-28 18:43:08]

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_ntp.cc (right):

https://codereview.chromium.org/17526008/diff/47014/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:57: instant_url_("Needs To Be Set") {}
On 2013/06/28 18:38:22, annark1 wrote:
> The reason this is here is because on start up the NavigationEntryCommitted
> function seems to get fired twice, and in those cases the load_details'
> previous_url is blank. Is there some dummy value I can put in for these types
of
> cases, so there's no chance of it seeming like there is a match when in fact
> there isn't?
> 
> On 2013/06/28 16:24:17, Jered wrote:
> > Omit this default value.
> 

How about not emitting the count when previous_url is blank? You don't want to
count 0 hovers when the ntp is initially committed, after all.

https://codereview.chromium.org/17526008/diff/58001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_ntp.cc (right):

https://codereview.chromium.org/17526008/diff/58001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:98: 
Put
// static
on the line above class static function definitions.

[Alexei Svitkine (slow), 2013-06-28 18:50:28]

https://codereview.chromium.org/17526008/diff/58001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_ntp.cc (right):

https://codereview.chromium.org/17526008/diff/58001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:15: 
Nit: Remove extra blank line here.

https://codereview.chromium.org/17526008/diff/58001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:17: namespace {
Nit: Add blank line after this.

https://codereview.chromium.org/17526008/diff/58001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:35: void emit_mouseover_count() {
This method isn't trivial so should use CamelCase convention for the name.

https://codereview.chromium.org/17526008/diff/58001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:61: std::string instant_url_;
Why not make this a GURL()? Also, remove the dummy value in the ctor, since the
default one should be fine. (You can then test .is_empty() to see if it was
initialized, if you need to.)

https://codereview.chromium.org/17526008/diff/58001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:100: if
(NTPLoggingUserData::FromWebContents(contents)) {
Suggestion, instead you can do this to avoid querying it twice:

if (NTPLoggingUserData* data = NTPLoggingUserData::FromWebContents(contents))
  data->increment_number_of_mouseovers();

Here and below.

[annark1, 2013-06-28 19:37:44]

Thanks for your comments.

https://codereview.chromium.org/17526008/diff/58001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_ntp.cc (right):

https://codereview.chromium.org/17526008/diff/58001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:15: 
On 2013/06/28 18:50:29, Alexei Svitkine wrote:
> Nit: Remove extra blank line here.

Done.

https://codereview.chromium.org/17526008/diff/58001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:17: namespace {
On 2013/06/28 18:50:29, Alexei Svitkine wrote:
> Nit: Add blank line after this.

Done.

https://codereview.chromium.org/17526008/diff/58001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:35: void emit_mouseover_count() {
On 2013/06/28 18:50:29, Alexei Svitkine wrote:
> This method isn't trivial so should use CamelCase convention for the name.

Done.

https://codereview.chromium.org/17526008/diff/58001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:61: std::string instant_url_;
On 2013/06/28 18:50:29, Alexei Svitkine wrote:
> Why not make this a GURL()? Also, remove the dummy value in the ctor, since
the
> default one should be fine. (You can then test .is_empty() to see if it was
> initialized, if you need to.)

Done.

https://codereview.chromium.org/17526008/diff/58001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:98: 
On 2013/06/28 18:43:09, Jered wrote:
> Put
> // static
> on the line above class static function definitions.

Done.

https://codereview.chromium.org/17526008/diff/58001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:100: if
(NTPLoggingUserData::FromWebContents(contents)) {
On 2013/06/28 18:50:29, Alexei Svitkine wrote:
> Suggestion, instead you can do this to avoid querying it twice:
> 
> if (NTPLoggingUserData* data = NTPLoggingUserData::FromWebContents(contents))
>   data->increment_number_of_mouseovers();
> 
> Here and below.

Done.

[Alexei Svitkine (slow), 2013-06-28 20:15:33]

lgtm with nits

considering adding a TEST= line that explain how to trigger this and see the
results on chrome://histograms

https://codereview.chromium.org/17526008/diff/64001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_ntp.cc (right):

https://codereview.chromium.org/17526008/diff/64001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:84: loader_.Init(GURL(instant_url()),
profile, active_tab, on_stale_callback);
Nit: Extract GURL(instant_url()) into a variable, since you use it in two places
in this function.

https://codereview.chromium.org/17526008/diff/64001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:103: contents)) {
Nit: Indent 4 more.

https://codereview.chromium.org/17526008/diff/64001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:111: contents)) {
Nit: Indent 4 more.

[Jered, 2013-06-28 20:19:30]

lgtm

Looks great! Please address Alexei's comments before submitting.

[annark1, 2013-06-28 20:47:26]

https://codereview.chromium.org/17526008/diff/64001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_ntp.cc (right):

https://codereview.chromium.org/17526008/diff/64001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:84: loader_.Init(GURL(instant_url()),
profile, active_tab, on_stale_callback);
On 2013/06/28 20:15:34, Alexei Svitkine wrote:
> Nit: Extract GURL(instant_url()) into a variable, since you use it in two
places
> in this function.

Done.

https://codereview.chromium.org/17526008/diff/64001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:103: contents)) {
On 2013/06/28 20:15:34, Alexei Svitkine wrote:
> Nit: Indent 4 more.

Done.

https://codereview.chromium.org/17526008/diff/64001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:111: contents)) {
On 2013/06/28 20:15:34, Alexei Svitkine wrote:
> Nit: Indent 4 more.

Done.

[beaudoin, 2013-07-04 16:12:03]

LGTM with a couple of nits.

https://codereview.chromium.org/17526008/diff/73001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_ntp.cc (right):

https://codereview.chromium.org/17526008/diff/73001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:47: virtual void
NavigationEntryCommitted OVERRIDE(
Nit: space between OVERRIDE and (

https://codereview.chromium.org/17526008/diff/73001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:116: }
Nit: Would be slightly easier to read and shorter as:
NTPLoggingUserData* data = NTPLoggingUserData::FromWebContents(contents);
if (data)
  data->increment_number_of_mouseovers();

https://codereview.chromium.org/17526008/diff/73001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:124: }
Same here.

https://codereview.chromium.org/17526008/diff/73001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/17526008/diff/73001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:1496:
SearchBox::Get(render_view)->CountMouseover();
In the else case we should probably DVLOG(1) that we got an unsupported
histogram name.

[annark1, 2013-07-04 19:25:08]

https://codereview.chromium.org/17526008/diff/73001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_ntp.cc (right):

https://codereview.chromium.org/17526008/diff/73001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:47: virtual void
NavigationEntryCommitted OVERRIDE(
On 2013/07/04 16:12:03, beaudoin wrote:
> Nit: space between OVERRIDE and (

Done.

https://codereview.chromium.org/17526008/diff/73001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:116: }
On 2013/07/04 16:12:03, beaudoin wrote:
> Nit: Would be slightly easier to read and shorter as:
> NTPLoggingUserData* data = NTPLoggingUserData::FromWebContents(contents);
> if (data)
>   data->increment_number_of_mouseovers();

Done.

https://codereview.chromium.org/17526008/diff/73001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.cc:124: }
On 2013/07/04 16:12:03, beaudoin wrote:
> Same here.

Done.

https://codereview.chromium.org/17526008/diff/73001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/17526008/diff/73001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:1496:
SearchBox::Get(render_view)->CountMouseover();
On 2013/07/04 16:12:03, beaudoin wrote:
> In the else case we should probably DVLOG(1) that we got an unsupported
> histogram name.

Done.

[commit-bot: I haz the power, 2013-07-08 14:46:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/annark@chromium.org/17526008/85006

[commit-bot: I haz the power, 2013-07-08 14:50:29]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2013-07-08 16:52:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/annark@chromium.org/17526008/104002

[commit-bot: I haz the power, 2013-07-08 18:47:17]

Change committed as 210398

[samarth, 2013-07-11 18:29:46]

https://chromiumcodereview.appspot.com/17526008/diff/104002/chrome/browser/ui...
File chrome/browser/ui/search/instant_ntp.h (right):

https://chromiumcodereview.appspot.com/17526008/diff/104002/chrome/browser/ui...
chrome/browser/ui/search/instant_ntp.h:45: // Used to log each time the user
mouses over NTP tiles or titles.
Sorry for the extremely late comments but please move this to InstantTab. 
InstantNTP is used to maintaing the _prerendered_ (invisible) NTP contents and
it doesn't make sense for it to be talking about hovers, etc.