Add enable-offlining-recent-pages feature switch.

Add enable-offlining-recent-pages feature switch. The feature is not yet implemented. See launch bug below for description.

BUG=587599
Committed: https://crrev.com/1b3ad9bddb26f0dedbc7463397867d0a9981f85b
Cr-Commit-Position: refs/heads/master@{#379997}

[Dmitry Titov, 2016-03-01 23:28:11]

Description was changed from

==========
Add enable-offline-last-n-pages feature switch.

BUG=587599
==========

to

==========
Add enable-offlining-recent-pages feature switch. The feature is not yet
implemented. See launch bug below for description.

BUG=587599
==========

[Dmitry Titov, 2016-03-02 00:38:25]

dimich@chromium.org changed reviewers:
+ jianli@chromium.org, rkaplow@chromium.org

[Dmitry Titov, 2016-03-02 00:38:26]

Hi, PTAL:

JianLi: all
RKaplow: histograms.xml

Thanks!

[jianli, 2016-03-02 00:52:32]

lgtm

https://codereview.chromium.org/1750883002/diff/60001/components/offline_page...
File components/offline_pages/offline_page_feature.cc (right):

https://codereview.chromium.org/1750883002/diff/60001/components/offline_page...
components/offline_pages/offline_page_feature.cc:75: bool hasDisableSwitch =
base::CommandLine::ForCurrentProcess()->HasSwitch(
nit: C++ style

[jianli, 2016-03-02 01:07:23]

Have you considered to switching to base::Feature API?

[rkaplow, 2016-03-03 18:34:24]

agreed - this mostly looks ok but if you can use the new feature API this could
simplify the logic:
https://code.google.com/p/chromium/codesearch#chromium/src/base/feature_list....

[Dmitry Titov, 2016-03-08 00:45:35]

PTAL 
switched to Feature API.

[jianli, 2016-03-08 00:48:57]

https://codereview.chromium.org/1750883002/diff/200001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1750883002/diff/200001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1804: {"offlining-recent-pages",
IDS_FLAGS_OFFLINING_RECENT_PAGES_NAME,
Do we still need this?

[Dmitry Titov, 2016-03-08 01:08:24]

https://codereview.chromium.org/1750883002/diff/200001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1750883002/diff/200001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1804: {"offlining-recent-pages",
IDS_FLAGS_OFFLINING_RECENT_PAGES_NAME,
On 2016/03/08 00:48:57, jianli wrote:
> Do we still need this?

I think so - that's what creates the entries in chrome://flags so users can
see/manage the features.

[jianli, 2016-03-08 01:08:29]

lgtm

[rkaplow, 2016-03-08 19:39:42]

https://codereview.chromium.org/1750883002/diff/200001/components/offline_pag...
File components/offline_pages/offline_page_feature.cc (right):

https://codereview.chromium.org/1750883002/diff/200001/components/offline_pag...
components/offline_pages/offline_page_feature.cc:82: return 
base::FeatureList::IsEnabled(kOffliningRecentPagesFeature) &&
this is a little bit confusing, because the isOfflinePagesEnabled then checks
the experimentation state.
One of the main points of the feature API is you don't need to manually check
the command line state and the field trial state anymore, the feature API
handles this:
https://code.google.com/p/chromium/codesearch#chromium/src/components/offline...

I guess this is just for offline pages which are recent, versus the full offline
pages experiment which is broader in scope.

SO the recent pages versus is on iff the experiment for offline pages is on
*and* the flag is set?

I'mn also a bit confused how you will be controlling how users are using this
feature - right now there's no experimentation control.

It seems it may make sense to put this as a experiment parameter in the
OfflinePages experiment. Then you can specify for each of your OfflinePages
groups which ones have the param set.

You can still leave the feature API setup you've already done to control the
state via flags - that is, if you really want users to manually opt into the
feature.

Does this overall make sense? Can ping me if you have questions.

[Dmitry Titov, 2016-03-08 20:52:06]

There is offline pages experiment that is going to ship soon. It already has
complicated experimental setup because it has a dependent feature in Zine that
is also separately controlled. As you said, it's broader in scope.

The idea of this flag is separate - I do not need to create sub-experiments with
main offline pages feature, especially considering it will be full shipped
likely before "recent" will be implemented.

I want a flag that will enable me to do development, to control the feature
manually from chrome://flags and command-line for development, testing and to
give other people to try.

Eventually we might want to do experiment or just enable it for shipping - this
is TBD. So for now, it is an independent flag protecting a feature in
development. It does depend on Offline Pages being available, that's why the
second check.

There is no Finch setup for this feature at the moment, it is not clear if we
will ever need one and in which shape. I'd plan to address it when it'll be
clear.

[rkaplow, 2016-03-08 22:10:48]

lgtm

makes sense. Just doing my experimental due diligence. 
thanks!

[Dmitry Titov, 2016-03-08 23:06:02]

The CQ bit was checked by dimich@chromium.org

[commit-bot: I haz the power, 2016-03-08 23:06:48]

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

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

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

[commit-bot: I haz the power, 2016-03-08 23:14:25]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Dmitry Titov, 2016-03-08 23:26:17]

The CQ bit was checked by dimich@chromium.org

[Dmitry Titov, 2016-03-08 23:26:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
rkaplow@chromium.org, jianli@chromium.org
Link to the patchset: https://codereview.chromium.org/1750883002/#ps220001
(title: "resolve")

[commit-bot: I haz the power, 2016-03-08 23:27:03]

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

[commit-bot: I haz the power, 2016-03-09 00:34:20]

Description was changed from

==========
Add enable-offlining-recent-pages feature switch. The feature is not yet
implemented. See launch bug below for description.

BUG=587599
==========

to

==========
Add enable-offlining-recent-pages feature switch. The feature is not yet
implemented. See launch bug below for description.

BUG=587599
==========

[commit-bot: I haz the power, 2016-03-09 00:34:21]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-03-09 00:35:42]

Description was changed from

==========
Add enable-offlining-recent-pages feature switch. The feature is not yet
implemented. See launch bug below for description.

BUG=587599
==========

to

==========
Add enable-offlining-recent-pages feature switch. The feature is not yet
implemented. See launch bug below for description.

BUG=587599

Committed: https://crrev.com/1b3ad9bddb26f0dedbc7463397867d0a9981f85b
Cr-Commit-Position: refs/heads/master@{#379997}
==========

[commit-bot: I haz the power, 2016-03-09 00:35:43]

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