Add FeatureList instance to renderers and propgate enable/disable flags.

Add FeatureList instance to renderers and propagate toggles.

Changes:
  - Moves --enable-features/--disable-features flags to content from
    chrome.
  - Copies their content when starting renderers.
  - In renderer_main.cc, creates and registers a FeatureList initialized
    with those flags.

BUG=526169
Committed: https://crrev.com/f10715e245f9ead26f15943b08ea871aa34083af
Cr-Commit-Position: refs/heads/master@{#351148}

[Alexei Svitkine (slow), 2015-09-18 14:59:01]

Patchset #1 (id:1) has been deleted

[Alexei Svitkine (slow), 2015-09-18 14:59:25]

asvitkine@chromium.org changed reviewers:
+ brettw@chromium.org, rkaplow@chromium.org

[rkaplow, 2015-09-21 14:22:17]

lgtm

[Alexei Svitkine (slow), 2015-09-21 20:33:26]

brettw: friendly ping

[brettw, 2015-09-21 23:13:50]

Sorry, spent free time today on the "6 chars crash chrome" bug. Will look
Tuesday.

[brettw, 2015-09-22 20:39:37]

Chatted offline and I suggested trying to keep the switch in Chrome by modifying
ContentRendererClient and AppendExtraCommandLineSwitches instead.

It seems like this same switch issue comes up from time-to-time. Can you add a
comment above the big list of switches that embedder switches can be added in
ContentBrowserClient::AppendExtraCommandLineSwitches which the embedded
implements (avoid mentioning "chrome" in content code).

[Alexei Svitkine (slow), 2015-09-22 22:13:48]

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

[Alexei Svitkine (slow), 2015-09-22 22:13:49]

+jam

I took a look at how we can do this without moving the flag to content.

However, the other challenge is initializing the FeatureList. We want to do this
early, so that code use the Feature API early as well - and not have to worry
about whether it has been initialized or not. I think it's good to have the
initialization to be done at the same place as FieldTrialList - so that folks
reasoning about these can treat the same for the purposes of when they can be
used.

Unfortunately, there's a couple of problems. One is there's no content callback
that's implemented in Chrome that runs this early. I looked at
ChromeContentRendererClient::RenderThreadStarted(), but I truly believe this is
too late - a lot of services have already started via RenderThreadImpl::Init() -
and I expect that some of them may want to use the Feature API in the future
(i.e. for the same reason that some of them already check command line flags).

The other issue is that FeatureList needs to be initialized for all of content
(not just content/ that's embedded in chrome/) - since content/ code will be
using that API. And per the design of the API, FeatureList needs to be fully
initialized before it is registered as the singleton instance (again, to avoid
races).

So, even if we do add a new API, in order to support these semantics, the
FeatureList* instance would need to be passed as a param to it, so that the
embedded can call feature_list->InitializeFromCommandLine(...) on it.

Does it sound reasonable to add this API?

[jam, 2015-09-23 18:21:39]

On 2015/09/22 22:13:49, Alexei Svitkine (slow) wrote:
> +jam
> 
> I took a look at how we can do this without moving the flag to content.
> 
> However, the other challenge is initializing the FeatureList. We want to do
this
> early, so that code use the Feature API early as well - and not have to worry
> about whether it has been initialized or not. I think it's good to have the
> initialization to be done at the same place as FieldTrialList - so that folks
> reasoning about these can treat the same for the purposes of when they can be
> used.
> 
> Unfortunately, there's a couple of problems. One is there's no content
callback
> that's implemented in Chrome that runs this early. I looked at
> ChromeContentRendererClient::RenderThreadStarted(), but I truly believe this
is
> too late - a lot of services have already started via RenderThreadImpl::Init()
-
> and I expect that some of them may want to use the Feature API in the future
> (i.e. for the same reason that some of them already check command line flags).
> 
> The other issue is that FeatureList needs to be initialized for all of content
> (not just content/ that's embedded in chrome/) - since content/ code will be
> using that API. And per the design of the API, FeatureList needs to be fully
> initialized before it is registered as the singleton instance (again, to avoid
> races).
> 
> So, even if we do add a new API, in order to support these semantics, the
> FeatureList* instance would need to be passed as a param to it, so that the
> embedded can call feature_list->InitializeFromCommandLine(...) on it.
> 
> Does it sound reasonable to add this API?

if base::FeatureList will be called from content, then lgtm. if we are only
using it in components or chrome, then there are other ways (let me know and i
can go into more detail)

[Alexei Svitkine (slow), 2015-09-23 19:32:01]

On 2015/09/23 18:21:39, jam wrote:
> On 2015/09/22 22:13:49, Alexei Svitkine (slow) wrote:
> > +jam
> > 
> > I took a look at how we can do this without moving the flag to content.
> > 
> > However, the other challenge is initializing the FeatureList. We want to do
> this
> > early, so that code use the Feature API early as well - and not have to
worry
> > about whether it has been initialized or not. I think it's good to have the
> > initialization to be done at the same place as FieldTrialList - so that
folks
> > reasoning about these can treat the same for the purposes of when they can
be
> > used.
> > 
> > Unfortunately, there's a couple of problems. One is there's no content
> callback
> > that's implemented in Chrome that runs this early. I looked at
> > ChromeContentRendererClient::RenderThreadStarted(), but I truly believe this
> is
> > too late - a lot of services have already started via
RenderThreadImpl::Init()
> -
> > and I expect that some of them may want to use the Feature API in the future
> > (i.e. for the same reason that some of them already check command line
flags).
> > 
> > The other issue is that FeatureList needs to be initialized for all of
content
> > (not just content/ that's embedded in chrome/) - since content/ code will be
> > using that API. And per the design of the API, FeatureList needs to be fully
> > initialized before it is registered as the singleton instance (again, to
avoid
> > races).
> > 
> > So, even if we do add a new API, in order to support these semantics, the
> > FeatureList* instance would need to be passed as a param to it, so that the
> > embedded can call feature_list->InitializeFromCommandLine(...) on it.
> > 
> > Does it sound reasonable to add this API?
> 
> if base::FeatureList will be called from content, then lgtm. if we are only
> using it in components or chrome, then there are other ways (let me know and i
> can go into more detail)

The intention is to support this capability - similar to how things in content/
can call FieldTrial APIs. Also, there's a lot of candidates in
content_switches.cc to migrate to the FeatureList API.

jam, to clarify - are you LGTM-ing the patch in its current state (where
FeatureList initialization is done entirely in content/ and flags are plumbed
through), or are you LGTM-ing my suggestion above - which is to keep the flags
in chrome/ and add an embedder API to initialize FeatureList that will be called
by content?

[Alexei Svitkine (slow), 2015-09-28 18:07:45]

Followed up-offline with jam@. He's ok with the current approach given
FeatureList API will be used by content/ code.

cq'ing

[Alexei Svitkine (slow), 2015-09-28 18:07:51]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2015-09-28 18:10:42]

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

[commit-bot: I haz the power, 2015-09-28 19:13:33]

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

[commit-bot: I haz the power, 2015-09-28 19:13:34]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Alexei Svitkine (slow), 2015-09-28 19:42:28]

The CQ bit was checked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2015-09-28 19:42:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
rkaplow@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1355613002/#ps60001
(title: "Remove kDisableFeatures from chrome_switches.h.")

[commit-bot: I haz the power, 2015-09-28 19:43:45]

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

[commit-bot: I haz the power, 2015-09-28 20:09:03]

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

[commit-bot: I haz the power, 2015-09-28 20:09:04]

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

[brettw, 2015-09-28 20:15:10]

rubberstamp lgtm based on jam's review (in case you needed owners for chrome or
something)

[Alexei Svitkine (slow), 2015-09-28 20:20:51]

Hmm, getting some:

"Failing build because ninja reported work to do.
This means that after completing a compile, another was run and
it resulted in still having work to do (that is, a no-op build
wasn't a no-op). Consult the first "ninja explain:" line for a
likely culprit."

Don't think it's related to this change. Will try again.

[Alexei Svitkine (slow), 2015-09-28 20:20:59]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2015-09-28 20:21:46]

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

[commit-bot: I haz the power, 2015-09-28 21:07:33]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-09-28 21:08:30]

Patchset 3 (id:??) landed as
https://crrev.com/f10715e245f9ead26f15943b08ea871aa34083af
Cr-Commit-Position: refs/heads/master@{#351148}