[Offline Pages on the NTP] Hide behind a base::Feature

[Offline Pages on the NTP] Hide behind a base::Feature

Specify "--enable-features=NTPOfflinePages" on the command line to enable the feature.

BUG=565219
Committed: https://crrev.com/45cf354bfb449c6510196a64882a8e0178ac5238
Cr-Commit-Position: refs/heads/master@{#369137}

[Marc Treib, 2016-01-08 14:40:34]

Description was changed from

==========
[Offline Pages on the NTP] Hide behind a base::Feature

BUG=565219
==========

to

==========
[Offline Pages on the NTP] Hide behind a base::Feature

Specify "--enable-features=NTPOfflinePages" on the command line to enable the
feature.

BUG=565219
==========

[Marc Treib, 2016-01-08 14:42:28]

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

[Marc Treib, 2016-01-08 14:42:28]

PTAL!
Since we probably won't get to launch this in M49, let's hide it behind a
Feature for now. (Probably a good idea anyway, since it gives us more
flexibility.)

[fgorski, 2016-01-08 17:21:40]

lgtm, but you should probably get a review from someone who knows more about the
feature list.

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

https://codereview.chromium.org/1565883006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:218:
private boolean ntpOfflinePagesEnabled() {
%s/ntpOfflinePagesEnabled/isNtpOfflinePagesEnabled/

[Marc Treib, 2016-01-11 10:37:30]

FeatureList is just a new, simpler API over FieldTrials/variations. I've used it
before.

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

https://codereview.chromium.org/1565883006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:218:
private boolean ntpOfflinePagesEnabled() {
On 2016/01/08 17:21:40, fgorski wrote:
> %s/ntpOfflinePagesEnabled/isNtpOfflinePagesEnabled/

Done.

[Marc Treib, 2016-01-11 10:37:51]

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

[Marc Treib, 2016-01-11 10:37:51]

+bauerb for OWNERS approval. PTAL!

[Bernhard Bauer, 2016-01-11 14:26:26]

Neat! LGTM

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

https://codereview.chromium.org/1565883006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:218:
private boolean isNtpOfflinePagesEnabled() {
Not that it makes a big difference here, but you could make this static?

[Marc Treib, 2016-01-11 14:35:24]

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

https://codereview.chromium.org/1565883006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:218:
private boolean isNtpOfflinePagesEnabled() {
On 2016/01/11 14:26:26, Bernhard Bauer wrote:
> Not that it makes a big difference here, but you could make this static?

Huh, turns out I can't:
../chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:218:
error: Illegal static declaration in inner class <anonymous
org.chromium.chrome.browser.ntp.NewTabPage$2>
        private static boolean isNtpOfflinePagesEnabled() {
                               ^
  modifier 'static' is only allowed in constant variable declarations

Apparently inner classes in Java can't have static methods. I suppose I could
move the method into the outer class?

[Bernhard Bauer, 2016-01-11 14:56:21]

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

https://codereview.chromium.org/1565883006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:218:
private boolean isNtpOfflinePagesEnabled() {
On 2016/01/11 14:35:23, Marc Treib wrote:
> On 2016/01/11 14:26:26, Bernhard Bauer wrote:
> > Not that it makes a big difference here, but you could make this static?
> 
> Huh, turns out I can't:
>
../chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:218:
> error: Illegal static declaration in inner class <anonymous
> org.chromium.chrome.browser.ntp.NewTabPage$2>
>         private static boolean isNtpOfflinePagesEnabled() {
>                                ^
>   modifier 'static' is only allowed in constant variable declarations
> 
> Apparently inner classes in Java can't have static methods. I suppose I could
> move the method into the outer class?

Huh. That is fine with me, I don't really feel strongly about it either way, so
you could also keep it like this.

[Marc Treib, 2016-01-13 10:18:55]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-01-13 10:18:55]

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

[commit-bot: I haz the power, 2016-01-13 10:19:18]

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

[commit-bot: I haz the power, 2016-01-13 10:59:43]

Description was changed from

==========
[Offline Pages on the NTP] Hide behind a base::Feature

Specify "--enable-features=NTPOfflinePages" on the command line to enable the
feature.

BUG=565219
==========

to

==========
[Offline Pages on the NTP] Hide behind a base::Feature

Specify "--enable-features=NTPOfflinePages" on the command line to enable the
feature.

BUG=565219
==========

[commit-bot: I haz the power, 2016-01-13 10:59:43]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-13 11:01:11]

Description was changed from

==========
[Offline Pages on the NTP] Hide behind a base::Feature

Specify "--enable-features=NTPOfflinePages" on the command line to enable the
feature.

BUG=565219
==========

to

==========
[Offline Pages on the NTP] Hide behind a base::Feature

Specify "--enable-features=NTPOfflinePages" on the command line to enable the
feature.

BUG=565219

Committed: https://crrev.com/45cf354bfb449c6510196a64882a8e0178ac5238
Cr-Commit-Position: refs/heads/master@{#369137}
==========

[commit-bot: I haz the power, 2016-01-13 11:01:12]

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