Emit correct NewTabPage.SuggestionsType metrics

Emit correct NewTabPage.SuggestionsType metrics

Committed: https://crrev.com/fddc3e38906e4dd0028421ba542149025b161fe9
Cr-Commit-Position: refs/heads/master@{#364190}

[zmin, 2015-10-09 18:34:33]

zmin@chromium.org changed reviewers:
+ fserb@chromium.org

[zmin, 2015-10-09 19:36:50]

Fixing unit test compiler error on Mac

[fserb, 2015-10-13 14:34:40]

fserb@chromium.org changed reviewers:
+ kmadhusu@chromium.org

[fserb, 2015-10-13 14:34:40]

LGTM

https://codereview.chromium.org/1396143002/diff/20001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/1396143002/diff/20001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_tab_helper.cc:400:
this->LogMostVisitedItemsSource(items);
no need for "this->" here.

https://codereview.chromium.org/1396143002/diff/20001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_tab_helper.cc:405: NTPLoggingEventType event;
you can move it inside the for().

https://codereview.chromium.org/1396143002/diff/20001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_tab_helper.cc:406: base::TimeDelta time =
base::TimeDelta();
you can just pass 0 as time. I'm not even sure this TimeDelta is what is usually
passed.

https://codereview.chromium.org/1396143002/diff/20001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_tab_helper.cc:415: // all suggestions are
provided by client
nit: "." at the end of the comment.

[zmin, 2015-10-13 15:08:38]

https://codereview.chromium.org/1396143002/diff/20001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/1396143002/diff/20001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_tab_helper.cc:400:
this->LogMostVisitedItemsSource(items);
On 2015/10/13 14:34:40, fserb wrote:
> no need for "this->" here.

Done.

https://codereview.chromium.org/1396143002/diff/20001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_tab_helper.cc:405: NTPLoggingEventType event;
On 2015/10/13 14:34:40, fserb wrote:
> you can move it inside the for().

Talk with @fserb. Moving event and time into the for loop as they are only used
by for loop and  there isn't any performance issue here.

https://codereview.chromium.org/1396143002/diff/20001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_tab_helper.cc:406: base::TimeDelta time =
base::TimeDelta();
On 2015/10/13 14:34:40, fserb wrote:
> you can just pass 0 as time. I'm not even sure this TimeDelta is what is
usually
> passed.

Talk with @fserb, 0 is the default value of TimeDelta and there is no ctor with
parameters. So no change here

https://codereview.chromium.org/1396143002/diff/20001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_tab_helper.cc:415: // all suggestions are
provided by client
On 2015/10/13 14:34:40, fserb wrote:
> nit: "." at the end of the comment.

Done.

[fserb, 2015-10-19 16:51:31]

fserb@chromium.org changed reviewers:
+ jered@chromium.org

[kmadhusu, 2015-12-07 23:03:37]

LGTM (Owners check).

Thanks.

[zmin, 2015-12-07 23:09:59]

zmin@chromium.org changed reviewers:
+ beaudoin@chromium.org
- jered@chromium.org

[zmin, 2015-12-07 23:09:59]

Hi Philippe,

Can you please review this changes?

Thanks,
Owen

[beaudoin, 2015-12-09 20:12:23]

On request for clarification, otherwise LGTM.

https://codereview.chromium.org/1396143002/diff/40001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/1396143002/diff/40001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_tab_helper.cc:415: this->OnLogEvent(event,
base::TimeDelta());
I'm not sure I understand the comment here. Do you mean you do not rely on
ntp_user_data_logger because it doesn't support what you want to do?

Please clarify.

[zmin, 2015-12-09 20:55:34]

https://codereview.chromium.org/1396143002/diff/40001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/1396143002/diff/40001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_tab_helper.cc:415: this->OnLogEvent(event,
base::TimeDelta());
On 2015/12/09 20:12:23, beaudoin wrote:
> I'm not sure I understand the comment here. Do you mean you do not rely on
> ntp_user_data_logger because it doesn't support what you want to do?
> 
> Please clarify.

Discussed with @beaudoin offline, will update the comments to make it more
clear.

[zmin, 2015-12-09 21:34:47]

The CQ bit was checked by zmin@chromium.org

[zmin, 2015-12-09 21:34:47]

The patchset sent to the CQ was uploaded after l-g-t-m from fserb@chromium.org,
beaudoin@chromium.org, kmadhusu@chromium.org
Link to the patchset: https://codereview.chromium.org/1396143002/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-12-09 21:41:25]

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

[commit-bot: I haz the power, 2015-12-09 22:59:16]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-09 23:00:01]

Description was changed from

==========
Emit correct NewTabPage.SuggestionsType metrics
==========

to

==========
Emit correct NewTabPage.SuggestionsType metrics

Committed: https://crrev.com/fddc3e38906e4dd0028421ba542149025b161fe9
Cr-Commit-Position: refs/heads/master@{#364190}
==========

[commit-bot: I haz the power, 2015-12-09 23:00:03]

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