Flag for Background Loading of Offline Pages.

Flag for Background Loading of Offline Pages.

This is a flag for the experimental background loading feature of
offline pages.  It defaults to off.

BUG=

Committed: https://crrev.com/468bbb93164bfede8ed6f1e5412a3893d3078070
Cr-Commit-Position: refs/heads/master@{#381334}

[Pete Williamson, 2016-03-08 23:59:21]

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

[fgorski, 2016-03-09 18:00:05]

Could you run this change by jianli?

https://codereview.chromium.org/1772233003/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1772233003/diff/1/chrome/browser/about_flags....
chrome/browser/about_flags.cc:534: switches::kEnableOfflinePagesAsBookmarks,
""},
??

https://codereview.chromium.org/1772233003/diff/1/chrome/browser/about_flags....
chrome/browser/about_flags.cc:536: switches::kEnableOfflinePagesAsSavedPages,
""},
??

https://codereview.chromium.org/1772233003/diff/1/chrome/browser/chrome_conte...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1772233003/diff/1/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:1640:
switches::kEnableOfflinePagesBackgroundLoading,
what is this one for?

https://codereview.chromium.org/1772233003/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_feature.h (right):

https://codereview.chromium.org/1772233003/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_feature.h:30: bool
IsOfflinePagesBackgroundLoadingEnabled();
document

[Pete Williamson, 2016-03-09 20:36:32]

petewil@chromium.org changed reviewers:
+ jianli@chromium.org
- fgorski@chromium.org

[Pete Williamson, 2016-03-09 20:36:33]

CR changes per FGorski@.   JianLi@, can you please take a look?

https://codereview.chromium.org/1772233003/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1772233003/diff/1/chrome/browser/about_flags....
chrome/browser/about_flags.cc:534: switches::kEnableOfflinePagesAsBookmarks,
""},
fixed the copy-paste bug with the switch name

https://codereview.chromium.org/1772233003/diff/1/chrome/browser/about_flags....
chrome/browser/about_flags.cc:536: switches::kEnableOfflinePagesAsSavedPages,
""},
On 2016/03/09 18:00:04, fgorski wrote:
> ??

Done.

https://codereview.chromium.org/1772233003/diff/1/chrome/browser/chrome_conte...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1772233003/diff/1/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:1640:
switches::kEnableOfflinePagesBackgroundLoading,
On 2016/03/09 18:00:04, fgorski wrote:
> what is this one for?

This line adds our new switch to the list of switches displayed on the
about:flags page.

https://codereview.chromium.org/1772233003/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_feature.h (right):

https://codereview.chromium.org/1772233003/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_feature.h:30: bool
IsOfflinePagesBackgroundLoadingEnabled();
On 2016/03/09 18:00:04, fgorski wrote:
> document

Done.

[fgorski, 2016-03-09 20:45:12]

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

[fgorski, 2016-03-09 20:45:13]

lgtm

https://codereview.chromium.org/1772233003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java
(right):

https://codereview.chromium.org/1772233003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java:265:
mOfflinePageBridge.isBackgroundLoadingEnabled());
align to 8 spaces.

https://codereview.chromium.org/1772233003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java:272:
mOfflinePageBridge.isBackgroundLoadingEnabled());
same here

[Pete Williamson, 2016-03-09 20:51:15]

Adjusted code alignment.

https://codereview.chromium.org/1772233003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java
(right):

https://codereview.chromium.org/1772233003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java:265:
mOfflinePageBridge.isBackgroundLoadingEnabled());
On 2016/03/09 20:45:13, fgorski wrote:
> align to 8 spaces.

Done.

https://codereview.chromium.org/1772233003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java:272:
mOfflinePageBridge.isBackgroundLoadingEnabled());
On 2016/03/09 20:45:13, fgorski wrote:
> same here

Done.

[dougarnett, 2016-03-09 21:59:57]

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

[dougarnett, 2016-03-09 21:59:57]

https://codereview.chromium.org/1772233003/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1772233003/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:14362: +      <message
name="IDS_FLAGS_ENABLE_OFFLINE_PAGE_BACKGROUND_LOADING" desc= "Option to enable
offline pages to be saved by a background page">
Not used?

[Pete Williamson, 2016-03-09 22:35:05]

CR fixes per dougarnet

https://codereview.chromium.org/1772233003/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1772233003/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:14362: +      <message
name="IDS_FLAGS_ENABLE_OFFLINE_PAGE_BACKGROUND_LOADING" desc= "Option to enable
offline pages to be saved by a background page">
On 2016/03/09 21:59:57, dougarnett wrote:
> Not used?

Correct.  I removed that message.

[dougarnett, 2016-03-09 23:22:51]

dougarnett@chromium.org changed reviewers:
- dougarnett@chromium.org

[dougarnett, 2016-03-09 23:22:52]

lgtm

[jianli, 2016-03-11 00:25:00]

Please switch to using base::Feature API (see
https://codereview.chromium.org/1750883002/ for example).

[Pete Williamson, 2016-03-11 23:01:53]

On 2016/03/11 00:25:00, jianli wrote:
> Please switch to using base::Feature API (see
> https://codereview.chromium.org/1750883002/ for example).

Done.

[Pete Williamson, 2016-03-11 23:03:19]

Changed to use the 'FeatureList' way of adding a flag.  JianLi, PTAL and see if
this is what you had in mind.

I got rid of one Java test because I couldn't figure out the right way to set
the feature flag from Java source code, if anybody knows, I'd be happy to add
that test back in, but I'm not too worried since the C++ test covers that case
already.

[jianli, 2016-03-12 00:08:30]

lgtm

https://codereview.chromium.org/1772233003/diff/80001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1772233003/diff/80001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1624: {"offline-pages-backgorund_loading_mode",
nit: replace "_" with '-". Also you can drop "mode" since this is just enable or
disable.

[Pete Williamson, 2016-03-12 00:48:29]

https://codereview.chromium.org/1772233003/diff/80001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1772233003/diff/80001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1624: {"offline-pages-backgorund_loading_mode",
On 2016/03/12 00:08:30, jianli wrote:
> nit: replace "_" with '-". Also you can drop "mode" since this is just enable
or
> disable.

Done.

[Pete Williamson, 2016-03-12 00:48:41]

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

[commit-bot: I haz the power, 2016-03-12 00:49:22]

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

[commit-bot: I haz the power, 2016-03-12 03:20:35]

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

[commit-bot: I haz the power, 2016-03-12 03:20:35]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Alexei Svitkine (slow), 2016-03-14 15:42:47]

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

[Alexei Svitkine (slow), 2016-03-14 15:42:48]

https://codereview.chromium.org/1772233003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/1772233003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:545:
private static native boolean nativeIsOfflinePagesBackgroundLoadingEnabled();
Optional nit: Up to you, but instead of adding the wrapper, you can add your
feature to here:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/and...

And then you can query it via ChromeFeatureList.java API. But your current set
up is OK too.

https://codereview.chromium.org/1772233003/diff/100001/chrome/browser/android...
File chrome/browser/android/offline_pages/offline_page_bridge.cc (right):

https://codereview.chromium.org/1772233003/diff/100001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_bridge.cc:80: JNIEnv* env,
const JavaParamRef<jclass>& clazz) {
Nit: 1 param per line when the first one wraps.

[Pete Williamson, 2016-03-14 17:36:35]

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

[commit-bot: I haz the power, 2016-03-14 17:36:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1772233003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1772233003/120001

[Pete Williamson, 2016-03-14 17:36:52]

Fixes for ASvitkine.

https://codereview.chromium.org/1772233003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/1772233003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:545:
private static native boolean nativeIsOfflinePagesBackgroundLoadingEnabled();
On 2016/03/14 15:42:48, Alexei Svitkine (very slow) wrote:
> Optional nit: Up to you, but instead of adding the wrapper, you can add your
> feature to here:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/and...
> 
> And then you can query it via ChromeFeatureList.java API. But your current set
> up is OK too.

Done.

https://codereview.chromium.org/1772233003/diff/100001/chrome/browser/android...
File chrome/browser/android/offline_pages/offline_page_bridge.cc (right):

https://codereview.chromium.org/1772233003/diff/100001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_bridge.cc:80: JNIEnv* env,
const JavaParamRef<jclass>& clazz) {
On 2016/03/14 15:42:48, Alexei Svitkine (very slow) wrote:
> Nit: 1 param per line when the first one wraps.

Done.

[Alexei Svitkine (slow), 2016-03-14 18:11:18]

lgtm

[commit-bot: I haz the power, 2016-03-14 20:03:51]

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

[commit-bot: I haz the power, 2016-03-14 20:03:52]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Pete Williamson, 2016-03-14 20:29:43]

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

[commit-bot: I haz the power, 2016-03-14 20:30:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1772233003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1772233003/140001

[Pete Williamson, 2016-03-14 20:31:27]

petewil@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Pete Williamson, 2016-03-14 20:31:28]

Ted:  Please do an OWNERS review for chrome_feature_list file, others have
already been reviewed.  Thanks!

[Ted C, 2016-03-14 22:16:41]

owners lgtm w/ test comment (non-blocking)

https://codereview.chromium.org/1772233003/diff/140001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java
(right):

https://codereview.chromium.org/1772233003/diff/140001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java:262:
public void testBackgroundLoadSwitch() throws Exception {
To be safe, wouldn't you want to add something like:

@CommandLineFlags.Add("disable-features=offline-pages-background-loading")

Seems like this test will fail as you change the default finch option, but maybe
you're fine with that.

[commit-bot: I haz the power, 2016-03-14 23:02:35]

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

[commit-bot: I haz the power, 2016-03-14 23:02:36]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Pete Williamson, 2016-03-15 21:17:21]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2016-03-15 21:17:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
fgorski@chromium.org, dougarnett@chromium.org, jianli@chromium.org,
asvitkine@chromium.org, tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/1772233003/#ps220001
(title: "Put @VisibleForTesting in the right place.")

[commit-bot: I haz the power, 2016-03-15 21:17:32]

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

[commit-bot: I haz the power, 2016-03-15 22:40:23]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-03-15 22:41:42]

Description was changed from

==========
Flag for Background Loading of Offline Pages.

This is a flag for the experimental background loading feature of
offline pages.  It defaults to off.

BUG=
==========

to

==========
Flag for Background Loading of Offline Pages.

This is a flag for the experimental background loading feature of
offline pages.  It defaults to off.

BUG=

Committed: https://crrev.com/468bbb93164bfede8ed6f1e5412a3893d3078070
Cr-Commit-Position: refs/heads/master@{#381334}
==========

[commit-bot: I haz the power, 2016-03-15 22:41:43]

Patchset 12 (id:??) landed as
https://crrev.com/468bbb93164bfede8ed6f1e5412a3893d3078070
Cr-Commit-Position: refs/heads/master@{#381334}