[NTP Snippets] Make a base::Feature instead of raw cmdline flag

[NTP Snippets] Make a base::Feature instead of raw cmdline flag
which is the new way to do things, and will make future field trials easy.
Also add an about:flags entry for ease of testing.

BUG=none

Committed: https://crrev.com/58464e93cd7a29bface90f220367ff55f85b883e
Cr-Commit-Position: refs/heads/master@{#377266}

[Marc Treib, 2016-02-09 13:56:37]

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

[Bernhard Bauer, 2016-02-09 14:44:20]

LGTM

https://codereview.chromium.org/1679223003/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/1679223003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:643: if
(ChromeFeatureList.isEnabled("NTPSnippets")) {
It seems like this should also get extract to a string constant. Would it make
sense to move this to a class like ChromeFeatures?

[Marc Treib, 2016-02-10 11:18:40]

https://codereview.chromium.org/1679223003/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/1679223003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:643: if
(ChromeFeatureList.isEnabled("NTPSnippets")) {
On 2016/02/09 14:44:20, Bernhard Bauer wrote:
> It seems like this should also get extract to a string constant. Would it make
> sense to move this to a class like ChromeFeatures?

I've added a constant to the existing ChromeFeatureList class - that's what you
meant, right? (We *could* of course make a method to retrieve the actual string
from native instead of duplicating it, but that doesn't seem worth a
Java->native roundtrip - WDYT?)

[Bernhard Bauer, 2016-02-10 11:24:56]

https://codereview.chromium.org/1679223003/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/1679223003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:643: if
(ChromeFeatureList.isEnabled("NTPSnippets")) {
On 2016/02/10 11:18:40, Marc Treib wrote:
> On 2016/02/09 14:44:20, Bernhard Bauer wrote:
> > It seems like this should also get extract to a string constant. Would it
make
> > sense to move this to a class like ChromeFeatures?
> 
> I've added a constant to the existing ChromeFeatureList class - that's what
you
> meant, right?

Pretty much -- although I was more thinking of putting it into a separate class,
in the same way that CommandLine and ChromeSwitches are different classes. But
this is fine.

> (We *could* of course make a method to retrieve the actual string
> from native instead of duplicating it, but that doesn't seem worth a
> Java->native roundtrip - WDYT?)

Yeah, I agree. If anything, we could look into automatically generating the Java
strings from the C++ strings, like we do for enums.

[Bernhard Bauer, 2016-02-10 17:14:21]

On 2016/02/10 11:24:56, Bernhard Bauer wrote:
>
https://codereview.chromium.org/1679223003/diff/1/chrome/android/java/src/org...
> File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
> (right):
> 
>
https://codereview.chromium.org/1679223003/diff/1/chrome/android/java/src/org...
> chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:643:
if
> (ChromeFeatureList.isEnabled("NTPSnippets")) {
> On 2016/02/10 11:18:40, Marc Treib wrote:
> > On 2016/02/09 14:44:20, Bernhard Bauer wrote:
> > > It seems like this should also get extract to a string constant. Would it
> make
> > > sense to move this to a class like ChromeFeatures?
> > 
> > I've added a constant to the existing ChromeFeatureList class - that's what
> you
> > meant, right?
> 
> Pretty much -- although I was more thinking of putting it into a separate
class,
> in the same way that CommandLine and ChromeSwitches are different classes. But
> this is fine.
> 
> > (We *could* of course make a method to retrieve the actual string
> > from native instead of duplicating it, but that doesn't seem worth a
> > Java->native roundtrip - WDYT?)
> 
> Yeah, I agree. If anything, we could look into automatically generating the
Java
> strings from the C++ strings, like we do for enums.

In related news: https://codereview.chromium.org/1664113002/

[Marc Treib, 2016-02-16 14:52:03]

Description was changed from

==========
Make NTPSnippets a base::Feature
which is the new way to do things, and will make future field trials easy.
Also add an about:flags entry for ease of testing.

BUG=none
==========

to

==========
[NTP Snippets] Make a base::Feature instead of raw cmdline flag
which is the new way to do things, and will make future field trials easy.
Also add an about:flags entry for ease of testing.

BUG=none
==========

[Marc Treib, 2016-02-24 10:34:17]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-02-24 10:34:17]

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

[commit-bot: I haz the power, 2016-02-24 10:35:07]

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

[commit-bot: I haz the power, 2016-02-24 11:34:52]

Description was changed from

==========
[NTP Snippets] Make a base::Feature instead of raw cmdline flag
which is the new way to do things, and will make future field trials easy.
Also add an about:flags entry for ease of testing.

BUG=none
==========

to

==========
[NTP Snippets] Make a base::Feature instead of raw cmdline flag
which is the new way to do things, and will make future field trials easy.
Also add an about:flags entry for ease of testing.

BUG=none
==========

[commit-bot: I haz the power, 2016-02-24 11:34:53]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-24 11:36:04]

Description was changed from

==========
[NTP Snippets] Make a base::Feature instead of raw cmdline flag
which is the new way to do things, and will make future field trials easy.
Also add an about:flags entry for ease of testing.

BUG=none
==========

to

==========
[NTP Snippets] Make a base::Feature instead of raw cmdline flag
which is the new way to do things, and will make future field trials easy.
Also add an about:flags entry for ease of testing.

BUG=none

Committed: https://crrev.com/58464e93cd7a29bface90f220367ff55f85b883e
Cr-Commit-Position: refs/heads/master@{#377266}
==========

[commit-bot: I haz the power, 2016-02-24 11:36:05]

Patchset 6 (id:??) landed as
https://crrev.com/58464e93cd7a29bface90f220367ff55f85b883e
Cr-Commit-Position: refs/heads/master@{#377266}