Add new feature flag to enable background loader for downloads

Add new feature flag to enable background loader for downloads

BUG=

Committed: https://crrev.com/936aa9ea40216e8f196671817773450235a1fe4f
Cr-Commit-Position: refs/heads/master@{#413242}

[chili, 2016-08-18 23:17:10]

chili@chromium.org changed reviewers:
+ dimich@chromium.org, dougarnett@chromium.org, petewil@chromium.org

[chili, 2016-08-18 23:17:10]

I had a little bit of trouble coming up with non-confusing flag descriptions. 
Let me know if there's something I'm missing :)

[dougarnett, 2016-08-19 00:19:25]

No structural feedback - just wonder about better name.

https://codereview.chromium.org/2254343002/diff/1/chrome/browser/android/chro...
File chrome/browser/android/chrome_feature_list.cc (right):

https://codereview.chromium.org/2254343002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_feature_list.cc:53:
&offline_pages::kBackgroundLoaderForDownloadsFeature,
wondering about slight better name. per chat suggestion from dimich, might be
kUseBackgroundLoaderForDownloads* but I also wonder about
kAllowBackgroundLoadingForDownloads* since is not prescriptive on how the
Download request will be satisfied (merely one option).

[chili, 2016-08-19 00:34:10]

On 2016/08/19 00:19:25, dougarnett wrote:
> No structural feedback - just wonder about better name.
> 
>
https://codereview.chromium.org/2254343002/diff/1/chrome/browser/android/chro...
> File chrome/browser/android/chrome_feature_list.cc (right):
> 
>
https://codereview.chromium.org/2254343002/diff/1/chrome/browser/android/chro...
> chrome/browser/android/chrome_feature_list.cc:53:
> &offline_pages::kBackgroundLoaderForDownloadsFeature,
> wondering about slight better name. per chat suggestion from dimich, might be
> kUseBackgroundLoaderForDownloads* but I also wonder about
> kAllowBackgroundLoadingForDownloads* since is not prescriptive on how the
> Download request will be satisfied (merely one option).

Grammatically, I would prefer some sort of noun vs verb.  This is because we're
going to use verbs with it, like "enable background loader for downloads". 
Similarly, the method "IsBackgroundLoaderForDownloadsEnabled" (or even
IsUsingBackgroundLoaderForDownloads) sounds better than
"IsUseBackgroundLoaderForDownloadsEnabled"... hence I removed the 'use'

However, I don't have strong feelings, so can change it if you feel otherwise

[dougarnett, 2016-08-19 01:31:17]

lgtm

[Dmitry Titov, 2016-08-19 02:21:53]

lgtm

I think we can deviate from pattern IsFooEnabled(), but in this case it is very
consistent with the code around, so the right name :)

[chili, 2016-08-19 04:07:21]

Not going to wait for Pete, since he'll be out for the rest of the week :)

Thanks for the fast turnaround!

[chili, 2016-08-19 04:07:28]

The CQ bit was checked by chili@chromium.org

[commit-bot: I haz the power, 2016-08-19 04:08:00]

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-08-19 04:13:23]

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

[commit-bot: I haz the power, 2016-08-19 04:13:24]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[chili, 2016-08-19 06:40:12]

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

[chili, 2016-08-19 06:40:12]

+Ted for owner

[Ted C, 2016-08-19 16:23:26]

lgtm after you fix compilation

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

https://codereview.chromium.org/2254343002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_feature.cc:39:
IsBackgroundLoadingForDownloadsEnabled();
../../components/offline_pages/offline_page_feature.cc:39:10: error: use of
undeclared identifier 'IsBackgroundLoadingForDownloadsEnabled'; did you mean
'IsBackgroundLoaderForDownloadsEnabled'?

Looks like the name in the header is wrong vs what you have below

[chili, 2016-08-19 20:20:02]

Thanks!

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

https://codereview.chromium.org/2254343002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_feature.cc:39:
IsBackgroundLoadingForDownloadsEnabled();
On 2016/08/19 16:23:26, Ted C wrote:
> ../../components/offline_pages/offline_page_feature.cc:39:10: error: use of
> undeclared identifier 'IsBackgroundLoadingForDownloadsEnabled'; did you mean
> 'IsBackgroundLoaderForDownloadsEnabled'?
> 
> Looks like the name in the header is wrong vs what you have below

Oops. Thanks for the catch!

[chili, 2016-08-19 20:20:13]

The CQ bit was checked by chili@chromium.org

[chili, 2016-08-19 20:20:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, dougarnett@chromium.org, dimich@chromium.org
Link to the patchset: https://codereview.chromium.org/2254343002/#ps20001
(title: "Fix spelling discrepancies")

[commit-bot: I haz the power, 2016-08-19 20:20:55]

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-08-19 21:25:31]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-19 21:33:58]

Description was changed from

==========
Add new feature flag to enable background loader for downloads

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

to

==========
Add new feature flag to enable background loader for downloads

BUG=

Committed: https://crrev.com/936aa9ea40216e8f196671817773450235a1fe4f
Cr-Commit-Position: refs/heads/master@{#413242}
==========

[commit-bot: I haz the power, 2016-08-19 21:33:59]

Patchset 2 (id:??) landed as
https://crrev.com/936aa9ea40216e8f196671817773450235a1fe4f
Cr-Commit-Position: refs/heads/master@{#413242}