Enable Popular Sites in Android demo mode.

Enable Popular Sites in Android demo mode.

BUG=625033
Committed: https://crrev.com/65f0f9bcaabb68512ce76b923bc9f3305b235b8d
Cr-Commit-Position: refs/heads/master@{#408100}

[Bernhard Bauer, 2016-07-22 09:53:43]

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

[commit-bot: I haz the power, 2016-07-22 09:54:03]

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-22 10:06:37]

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

[commit-bot: I haz the power, 2016-07-22 10:06:38]

Dry run: Try jobs failed on following builders:
  win8_chromium_gyp_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...)

[Bernhard Bauer, 2016-07-22 10:31:29]

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

[commit-bot: I haz the power, 2016-07-22 10:31:41]

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-22 11:28:34]

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

[commit-bot: I haz the power, 2016-07-22 11:28:34]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-07-22 12:47:35]

Description was changed from

==========
Enable Popular Sites in Android demo mode.

BUG=625033
==========

to

==========
Enable Popular Sites in Android demo mode.

BUG=625033
==========

[Bernhard Bauer, 2016-07-22 12:47:35]

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

[Bernhard Bauer, 2016-07-22 12:48:34]

Please review. Thanks!

[sfiera, 2016-07-22 12:55:45]

sfiera@chromium.org changed reviewers:
+ sfiera@chromium.org

[sfiera, 2016-07-22 12:55:46]

Files missing? Doesn't seem that this will do anything with just the .java
source. (fine for .cc changes to be in a separate CL, but this description of
this one implies a change in behavior)

[Marc Treib, 2016-07-22 12:59:03]

https://codereview.chromium.org/2172203002/diff/30001/components/ntp_tiles/an...
File
components/ntp_tiles/android/java/src/org/chromium/components/ntptiles/MostVisitedSites.java
(right):

https://codereview.chromium.org/2172203002/diff/30001/components/ntp_tiles/an...
components/ntp_tiles/android/java/src/org/chromium/components/ntptiles/MostVisitedSites.java:19:
private static boolean isPopularSitesEnabled() {
As Chris says, this isn't used anywhere.

The name doesn't make it clear if this will be and'ed or or'ed with the other
"enabled" check(s). isPopularSitesForceEnabled maybe?

[sfiera, 2016-07-22 13:01:48]

https://codereview.chromium.org/2172203002/diff/30001/components/ntp_tiles/an...
File
components/ntp_tiles/android/java/src/org/chromium/components/ntptiles/MostVisitedSites.java
(right):

https://codereview.chromium.org/2172203002/diff/30001/components/ntp_tiles/an...
components/ntp_tiles/android/java/src/org/chromium/components/ntptiles/MostVisitedSites.java:19:
private static boolean isPopularSitesEnabled() {
On 2016/07/22 12:59:03, Marc Treib wrote:
> As Chris says, this isn't used anywhere.
> 
> The name doesn't make it clear if this will be and'ed or or'ed with the other
> "enabled" check(s). isPopularSitesForceEnabled maybe?

Or isDemoUser() and let the C++ code figure it out. In the long run, Popular
Sites will be enabled both in and out of demo mode, no?

[Bernhard Bauer, 2016-07-22 14:03:49]

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

[commit-bot: I haz the power, 2016-07-22 14:04:05]

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

[Bernhard Bauer, 2016-07-22 14:11:57]

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

[commit-bot: I haz the power, 2016-07-22 14:12:12]

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

[Bernhard Bauer, 2016-07-22 14:37:38]

On 2016/07/22 12:55:46, sfiera wrote:
> Files missing? Doesn't seem that this will do anything with just the .java
> source. (fine for .cc changes to be in a separate CL, but this description of
> this one implies a change in behavior)

Durr, turns I'd been editing a pre-move copy of most_visited_sites.cc...

https://codereview.chromium.org/2172203002/diff/30001/components/ntp_tiles/an...
File
components/ntp_tiles/android/java/src/org/chromium/components/ntptiles/MostVisitedSites.java
(right):

https://codereview.chromium.org/2172203002/diff/30001/components/ntp_tiles/an...
components/ntp_tiles/android/java/src/org/chromium/components/ntptiles/MostVisitedSites.java:19:
private static boolean isPopularSitesEnabled() {
On 2016/07/22 12:59:03, Marc Treib wrote:
> As Chris says, this isn't used anywhere.
> 
> The name doesn't make it clear if this will be and'ed or or'ed with the other
> "enabled" check(s). isPopularSitesForceEnabled maybe?

Done.

https://codereview.chromium.org/2172203002/diff/30001/components/ntp_tiles/an...
components/ntp_tiles/android/java/src/org/chromium/components/ntptiles/MostVisitedSites.java:19:
private static boolean isPopularSitesEnabled() {
On 2016/07/22 13:01:48, sfiera wrote:
> On 2016/07/22 12:59:03, Marc Treib wrote:
> > As Chris says, this isn't used anywhere.
> > 
> > The name doesn't make it clear if this will be and'ed or or'ed with the
other
> > "enabled" check(s). isPopularSitesForceEnabled maybe?
> 
> Or isDemoUser() and let the C++ code figure it out. In the long run, Popular
> Sites will be enabled both in and out of demo mode, no?

Yes, but all that means is that this will probably be reverted anyway...

[commit-bot: I haz the power, 2016-07-22 15:06:43]

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

[commit-bot: I haz the power, 2016-07-22 15:06:44]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2016-07-25 12:38:09]

lgtm

[Bernhard Bauer, 2016-07-25 15:44:10]

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

[Bernhard Bauer, 2016-07-25 15:44:11]

Jochen, could I get an OWNERS review for the GYP changes? Thanks!

[jochen (gone - plz use gerrit), 2016-07-25 16:25:03]

lgtm

[Bernhard Bauer, 2016-07-27 11:18:02]

The CQ bit was checked by bauerb@chromium.org

[commit-bot: I haz the power, 2016-07-27 11:18:13]

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-27 12:15:33]

Description was changed from

==========
Enable Popular Sites in Android demo mode.

BUG=625033
==========

to

==========
Enable Popular Sites in Android demo mode.

BUG=625033
==========

[commit-bot: I haz the power, 2016-07-27 12:15:35]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2016-07-27 12:18:19]

Description was changed from

==========
Enable Popular Sites in Android demo mode.

BUG=625033
==========

to

==========
Enable Popular Sites in Android demo mode.

BUG=625033

Committed: https://crrev.com/65f0f9bcaabb68512ce76b923bc9f3305b235b8d
Cr-Commit-Position: refs/heads/master@{#408100}
==========

[commit-bot: I haz the power, 2016-07-27 12:18:21]

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

[klobag.chromium, 2016-07-29 22:59:45]

klobag@chromium.org changed reviewers:
+ klobag@chromium.org

[klobag.chromium, 2016-07-29 22:59:45]

Hmm, this seems complicated.

Why not just enable kEnableNTPPopularSites flag when demoUser is true?

[Bernhard Bauer, 2016-07-30 12:27:07]

On 2016/07/29 22:59:45, klobag.chromium wrote:
> Hmm, this seems complicated.

Is it? Most of the changes are to build files and JNI registration, because
we're adding Java code to the ntp_tiles component which had been
platform-independent so far.

> Why not just enable kEnableNTPPopularSites flag when demoUser is true?

We would have to ensure the command line is modified before Popular Sites is
initialized, so we would have to add the check to the startup path somewhere
before the native library is loaded, and accumulating random initialization code
during startup seems like an anti-pattern to me. (Then there is also the
anti-pattern of using the command line as general-purpose state.)

[klobag.chromium, 2016-08-01 19:21:41]

On 2016/07/30 12:27:07, Bernhard Bauer wrote:
> On 2016/07/29 22:59:45, klobag.chromium wrote:
> > Hmm, this seems complicated.
> 
> Is it? Most of the changes are to build files and JNI registration, because
> we're adding Java code to the ntp_tiles component which had been
> platform-independent so far.

The CL looks bigger from merging perspective. But agree it is simple from code
perspective.

> 
> > Why not just enable kEnableNTPPopularSites flag when demoUser is true?
> 
> We would have to ensure the command line is modified before Popular Sites is
> initialized, so we would have to add the check to the startup path somewhere
> before the native library is loaded, and accumulating random initialization
code
> during startup seems like an anti-pattern to me. (Then there is also the
> anti-pattern of using the command line as general-purpose state.)

I meant at start up before native initialization.