Cleanup: Change LogMostVisitedImpression|Navigation APIs to take an enum

Cleanup: Change LogMostVisitedImpression|Navigation APIs to take an enum
(instead of a string with only a few legal values)

BUG=625161
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

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

[Marc Treib, 2016-07-05 13:34:19]

Description was changed from

==========
Cleanup: Change LogMostVisitedImpression|Navigation APIs to take an enum
(instead of a string with only a few legal values)

BUG=625161
==========

to

==========
Cleanup: Change LogMostVisitedImpression|Navigation APIs to take an enum
(instead of a string with only a few legal values)

BUG=625161
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[Marc Treib, 2016-07-05 14:25:31]

treib@chromium.org changed reviewers:
+ bauerb@chromium.org

[Marc Treib, 2016-07-05 14:25:32]

PTAL!

[Bernhard Bauer, 2016-07-05 14:31:54]

https://codereview.chromium.org/2117373002/diff/1/chrome/browser/resources/lo...
File chrome/browser/resources/local_ntp/most_visited_single.js (right):

https://codereview.chromium.org/2117373002/diff/1/chrome/browser/resources/lo...
chrome/browser/resources/local_ntp/most_visited_single.js:56:
NTP_TILE_SOURCE_CLIENT: 0,
Nit: you get the equivalent of namespacing here -- no need to prefix these
values with NTP_TILE_SOURCE.

https://codereview.chromium.org/2117373002/diff/1/chrome/browser/resources/lo...
File chrome/browser/resources/local_ntp/most_visited_util.js (right):

https://codereview.chromium.org/2117373002/diff/1/chrome/browser/resources/lo...
chrome/browser/resources/local_ntp/most_visited_util.js:55: var
NTP_LOGGING_TILE_SOURCE = {
Why do we duplicate this again?

[Marc Treib, 2016-07-05 14:37:53]

https://codereview.chromium.org/2117373002/diff/1/chrome/browser/resources/lo...
File chrome/browser/resources/local_ntp/most_visited_single.js (right):

https://codereview.chromium.org/2117373002/diff/1/chrome/browser/resources/lo...
chrome/browser/resources/local_ntp/most_visited_single.js:56:
NTP_TILE_SOURCE_CLIENT: 0,
On 2016/07/05 14:31:54, Bernhard Bauer wrote:
> Nit: you get the equivalent of namespacing here -- no need to prefix these
> values with NTP_TILE_SOURCE.

I'd like to keep the entries identical to the C++ enum entries, to make them
greppable. LOG_TYPE above does the same. (Bad enough that the values are
duplicated in C++ and JS, but it should at least be possible to search for
them...)

https://codereview.chromium.org/2117373002/diff/1/chrome/browser/resources/lo...
File chrome/browser/resources/local_ntp/most_visited_util.js (right):

https://codereview.chromium.org/2117373002/diff/1/chrome/browser/resources/lo...
chrome/browser/resources/local_ntp/most_visited_util.js:55: var
NTP_LOGGING_TILE_SOURCE = {
On 2016/07/05 14:31:54, Bernhard Bauer wrote:
> Why do we duplicate this again?

most_visited_util.js is used by the old, multi-iframe NTP.
most_visited_single.js is the new, single-iframe version, which has copies of
this stuff rather than some common include. I don't know why - I guess the
original plan might have been to delete the old stuff at some point? Anyway,
that's not gonna happen, so all this should be moved to a common file.
Do you want that to happen in the same CL?

[Bernhard Bauer, 2016-07-05 14:43:42]

https://codereview.chromium.org/2117373002/diff/1/chrome/browser/resources/lo...
File chrome/browser/resources/local_ntp/most_visited_util.js (right):

https://codereview.chromium.org/2117373002/diff/1/chrome/browser/resources/lo...
chrome/browser/resources/local_ntp/most_visited_util.js:55: var
NTP_LOGGING_TILE_SOURCE = {
On 2016/07/05 14:37:53, Marc Treib wrote:
> On 2016/07/05 14:31:54, Bernhard Bauer wrote:
> > Why do we duplicate this again?
> 
> most_visited_util.js is used by the old, multi-iframe NTP.
> most_visited_single.js is the new, single-iframe version, which has copies of
> this stuff rather than some common include. I don't know why - I guess the
> original plan might have been to delete the old stuff at some point? Anyway,
> that's not gonna happen, so all this should be moved to a common file.
> Do you want that to happen in the same CL?

Up to you. A cleanup CL is not the worst place to make that change, but it could
also be a separate CL.

[Marc Treib, 2016-07-05 14:46:42]

https://codereview.chromium.org/2117373002/diff/1/chrome/browser/resources/lo...
File chrome/browser/resources/local_ntp/most_visited_util.js (right):

https://codereview.chromium.org/2117373002/diff/1/chrome/browser/resources/lo...
chrome/browser/resources/local_ntp/most_visited_util.js:55: var
NTP_LOGGING_TILE_SOURCE = {
On 2016/07/05 14:43:42, Bernhard Bauer wrote:
> On 2016/07/05 14:37:53, Marc Treib wrote:
> > On 2016/07/05 14:31:54, Bernhard Bauer wrote:
> > > Why do we duplicate this again?
> > 
> > most_visited_util.js is used by the old, multi-iframe NTP.
> > most_visited_single.js is the new, single-iframe version, which has copies
of
> > this stuff rather than some common include. I don't know why - I guess the
> > original plan might have been to delete the old stuff at some point? Anyway,
> > that's not gonna happen, so all this should be moved to a common file.
> > Do you want that to happen in the same CL?
> 
> Up to you. A cleanup CL is not the worst place to make that change, but it
could
> also be a separate CL.

Since that would be a fairly big and orthogonal change, and this CL is already
large enough, I'll go for followup CL. :)

[Bernhard Bauer, 2016-07-05 14:59:43]

lgtm

https://codereview.chromium.org/2117373002/diff/1/chrome/browser/resources/lo...
File chrome/browser/resources/local_ntp/most_visited_util.js (right):

https://codereview.chromium.org/2117373002/diff/1/chrome/browser/resources/lo...
chrome/browser/resources/local_ntp/most_visited_util.js:55: var
NTP_LOGGING_TILE_SOURCE = {
On 2016/07/05 14:46:42, Marc Treib wrote:
> On 2016/07/05 14:43:42, Bernhard Bauer wrote:
> > On 2016/07/05 14:37:53, Marc Treib wrote:
> > > On 2016/07/05 14:31:54, Bernhard Bauer wrote:
> > > > Why do we duplicate this again?
> > > 
> > > most_visited_util.js is used by the old, multi-iframe NTP.
> > > most_visited_single.js is the new, single-iframe version, which has copies
> of
> > > this stuff rather than some common include. I don't know why - I guess the
> > > original plan might have been to delete the old stuff at some point?
Anyway,
> > > that's not gonna happen, so all this should be moved to a common file.
> > > Do you want that to happen in the same CL?
> > 
> > Up to you. A cleanup CL is not the worst place to make that change, but it
> could
> > also be a separate CL.
> 
> Since that would be a fairly big and orthogonal change, and this CL is already
> large enough, I'll go for followup CL. :)

Add a TODO then? :)

[Marc Treib, 2016-07-05 15:10:16]

https://codereview.chromium.org/2117373002/diff/1/chrome/browser/resources/lo...
File chrome/browser/resources/local_ntp/most_visited_util.js (right):

https://codereview.chromium.org/2117373002/diff/1/chrome/browser/resources/lo...
chrome/browser/resources/local_ntp/most_visited_util.js:55: var
NTP_LOGGING_TILE_SOURCE = {
On 2016/07/05 14:59:42, Bernhard Bauer wrote:
> On 2016/07/05 14:46:42, Marc Treib wrote:
> > On 2016/07/05 14:43:42, Bernhard Bauer wrote:
> > > On 2016/07/05 14:37:53, Marc Treib wrote:
> > > > On 2016/07/05 14:31:54, Bernhard Bauer wrote:
> > > > > Why do we duplicate this again?
> > > > 
> > > > most_visited_util.js is used by the old, multi-iframe NTP.
> > > > most_visited_single.js is the new, single-iframe version, which has
copies
> > of
> > > > this stuff rather than some common include. I don't know why - I guess
the
> > > > original plan might have been to delete the old stuff at some point?
> Anyway,
> > > > that's not gonna happen, so all this should be moved to a common file.
> > > > Do you want that to happen in the same CL?
> > > 
> > > Up to you. A cleanup CL is not the worst place to make that change, but it
> > could
> > > also be a separate CL.
> > 
> > Since that would be a fairly big and orthogonal change, and this CL is
already
> > large enough, I'll go for followup CL. :)
> 
> Add a TODO then? :)

Done.

[Marc Treib, 2016-07-05 15:10:49]

treib@chromium.org changed reviewers:
+ dcheng@chromium.org

[Marc Treib, 2016-07-05 15:10:49]

+dcheng for render_messages.h. PTAL!

[dcheng, 2016-07-06 01:51:25]

LGTM with nits

https://codereview.chromium.org/2117373002/diff/20001/chrome/common/ntp_loggi...
File chrome/common/ntp_logging_events.h (right):

https://codereview.chromium.org/2117373002/diff/20001/chrome/common/ntp_loggi...
chrome/common/ntp_logging_events.h:56: enum NTPLoggingTileSource {
Nit: Can we make this a scoped enum, i.e. enum class NTPLoggingTileSource? Then
the enum members can just be CLIENT, SERVER, LAST.

https://codereview.chromium.org/2117373002/diff/20001/chrome/renderer/searchb...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/2117373002/diff/20001/chrome/renderer/searchb...
chrome/renderer/searchbox/searchbox_extension.cc:1070: if
(args[1]->Uint32Value() <= NTP_TILE_SOURCE_LAST) {
Nit: I have a weak preference for making this a DCHECK, since this should
"never" happen. If a renderer wants to shoot itself in the foot, then this
wouldn't really do anything anyway.

[Marc Treib, 2016-07-06 08:49:07]

https://codereview.chromium.org/2117373002/diff/20001/chrome/common/ntp_loggi...
File chrome/common/ntp_logging_events.h (right):

https://codereview.chromium.org/2117373002/diff/20001/chrome/common/ntp_loggi...
chrome/common/ntp_logging_events.h:56: enum NTPLoggingTileSource {
On 2016/07/06 01:51:25, dcheng wrote:
> Nit: Can we make this a scoped enum, i.e. enum class NTPLoggingTileSource?
Then
> the enum members can just be CLIENT, SERVER, LAST.

The enum values are duplicated in JavaScript code. To at least make them
greppable, I'd like to keep the long, unique names.
(If you know of a way to share an enum (or just some constants) between C++ and
JS, I'd love to hear it...)

https://codereview.chromium.org/2117373002/diff/20001/chrome/renderer/searchb...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/2117373002/diff/20001/chrome/renderer/searchb...
chrome/renderer/searchbox/searchbox_extension.cc:1070: if
(args[1]->Uint32Value() <= NTP_TILE_SOURCE_LAST) {
On 2016/07/06 01:51:25, dcheng wrote:
> Nit: I have a weak preference for making this a DCHECK, since this should
> "never" happen. If a renderer wants to shoot itself in the foot, then this
> wouldn't really do anything anyway.

But the same would apply for all the other validity checks in this file, such as
number and types of arguments.
I guess a ThrowInvalidParameters might be appropriate? TBH, I just copied the
NTPLoggingEventType parsing from above...

[Marc Treib, 2016-07-06 08:55:56]

https://codereview.chromium.org/2117373002/diff/20001/chrome/renderer/searchb...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/2117373002/diff/20001/chrome/renderer/searchb...
chrome/renderer/searchbox/searchbox_extension.cc:1070: if
(args[1]->Uint32Value() <= NTP_TILE_SOURCE_LAST) {
On 2016/07/06 08:49:07, Marc Treib wrote:
> On 2016/07/06 01:51:25, dcheng wrote:
> > Nit: I have a weak preference for making this a DCHECK, since this should
> > "never" happen. If a renderer wants to shoot itself in the foot, then this
> > wouldn't really do anything anyway.
> 
> But the same would apply for all the other validity checks in this file, such
as
> number and types of arguments.
> I guess a ThrowInvalidParameters might be appropriate? TBH, I just copied the
> NTPLoggingEventType parsing from above...

Hm, then again, looking at the code around, nothing seems to
ThrowInvalidParameters on invalid values. I'm not familiar enough with all this
to say what the Right Thing is, but I also think it won't make much of a
difference in practice.

[Bernhard Bauer, 2016-07-06 09:00:52]

https://codereview.chromium.org/2117373002/diff/20001/chrome/common/ntp_loggi...
File chrome/common/ntp_logging_events.h (right):

https://codereview.chromium.org/2117373002/diff/20001/chrome/common/ntp_loggi...
chrome/common/ntp_logging_events.h:56: enum NTPLoggingTileSource {
On 2016/07/06 08:49:06, Marc Treib wrote:
> On 2016/07/06 01:51:25, dcheng wrote:
> > Nit: Can we make this a scoped enum, i.e. enum class NTPLoggingTileSource?
> Then
> > the enum members can just be CLIENT, SERVER, LAST.
> 
> The enum values are duplicated in JavaScript code. To at least make them
> greppable, I'd like to keep the long, unique names.

BUT I WANTED TO USE THE SHORT VALUES IN JAVASCRIPT AS WELL 😉

If we always use them namespaced (which we have to do anyway), grepping should
be possible too, no? You'd just have to use different namespace separators for
JS vs C++.

> (If you know of a way to share an enum (or just some constants) between C++
and
> JS, I'd love to hear it...)

[dcheng, 2016-07-06 09:01:28]

https://codereview.chromium.org/2117373002/diff/20001/chrome/renderer/searchb...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/2117373002/diff/20001/chrome/renderer/searchb...
chrome/renderer/searchbox/searchbox_extension.cc:1070: if
(args[1]->Uint32Value() <= NTP_TILE_SOURCE_LAST) {
On 2016/07/06 08:55:56, Marc Treib wrote:
> On 2016/07/06 08:49:07, Marc Treib wrote:
> > On 2016/07/06 01:51:25, dcheng wrote:
> > > Nit: I have a weak preference for making this a DCHECK, since this should
> > > "never" happen. If a renderer wants to shoot itself in the foot, then this
> > > wouldn't really do anything anyway.
> > 
> > But the same would apply for all the other validity checks in this file,
such
> as
> > number and types of arguments.
> > I guess a ThrowInvalidParameters might be appropriate? TBH, I just copied
the
> > NTPLoggingEventType parsing from above...
> 
> Hm, then again, looking at the code around, nothing seems to
> ThrowInvalidParameters on invalid values. I'm not familiar enough with all
this
> to say what the Right Thing is, but I also think it won't make much of a
> difference in practice.

Shrug, if it's copy and pasted, I guess that's fine. I just personally prefer to
visibly error out--in particular, with a DCHECK, since this really is an assert
of a condition that should always be true--since hitting this case means the
developer has a typo or something.

[Marc Treib, 2016-07-06 10:14:38]

https://codereview.chromium.org/2117373002/diff/20001/chrome/common/ntp_loggi...
File chrome/common/ntp_logging_events.h (right):

https://codereview.chromium.org/2117373002/diff/20001/chrome/common/ntp_loggi...
chrome/common/ntp_logging_events.h:56: enum NTPLoggingTileSource {
On 2016/07/06 09:00:51, Bernhard Bauer wrote:
> On 2016/07/06 08:49:06, Marc Treib wrote:
> > On 2016/07/06 01:51:25, dcheng wrote:
> > > Nit: Can we make this a scoped enum, i.e. enum class NTPLoggingTileSource?
> > Then
> > > the enum members can just be CLIENT, SERVER, LAST.
> > 
> > The enum values are duplicated in JavaScript code. To at least make them
> > greppable, I'd like to keep the long, unique names.
> 
> BUT I WANTED TO USE THE SHORT VALUES IN JAVASCRIPT AS WELL 😉
> 
> If we always use them namespaced (which we have to do anyway), grepping should
> be possible too, no? You'd just have to use different namespace separators for
> JS vs C++.
> 
> > (If you know of a way to share an enum (or just some constants) between C++
> and
> > JS, I'd love to hear it...)
> 

Which is still a lot less convenient (and thus more error-prone) than just
searching for one string. But okay, I guess I'm outvoted here. Done.

[Marc Treib, 2016-07-06 10:17:30]

The CQ bit was checked by treib@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-06 10:17:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-06 10:51:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-06 10:51:36]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2016-07-06 10:56:37]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-07-06 10:56:37]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2117373002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-07-06 10:56:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-06 11:02:59]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-06 11:03:05]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-06 11:04:33]

Description was changed from

==========
Cleanup: Change LogMostVisitedImpression|Navigation APIs to take an enum
(instead of a string with only a few legal values)

BUG=625161
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Cleanup: Change LogMostVisitedImpression|Navigation APIs to take an enum
(instead of a string with only a few legal values)

BUG=625161
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

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

[commit-bot: I haz the power, 2016-07-06 11:04:34]

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