Register a default instance of FeatureList for content browser.

Register a default instance of FeatureList for content browser.

Normally, this is done in chrome_browser_main.cc in Chrome, but
given that we can't expect the embedder to necessarily initialize
the FeatureList, provide a default one if the embedder didn't.

This fixes an error when running Content Shell encountered by CLs
starting to use FeatureList, which didn't initialize the FeatureList
prior to this CL.

BUG=548289
Committed: https://crrev.com/9d96abfb3e5a5f0ed012e9104cf77cd382d2dd47
Cr-Commit-Position: refs/heads/master@{#357437}

[Alexei Svitkine (slow), 2015-10-28 20:11:12]

Description was changed from

==========
Register a default instance of FeatureList for content browser.

Normally, this is done in chrome_browser_main.cc in Chrome, but
given that we can't expect the embedder to necessarily initialize
the FeatureList, provide a default one if the embedder didn't
initialize theirs.

This fixes an error (encountered by CLs starting to use FeatureList)
when running Content Shell, which previously didn't initialize the
FeatureList.

BUG=548289
==========

to

==========
Register a default instance of FeatureList for content browser.

Normally, this is done in chrome_browser_main.cc in Chrome, but
given that we can't expect the embedder to necessarily initialize
the FeatureList, provide a default one if the embedder didn't.

This fixes an error (encountered by CLs starting to use FeatureList)
when running Content Shell, which previously didn't initialize the
FeatureList.

BUG=548289
==========

[Alexei Svitkine (slow), 2015-10-28 20:11:55]

Description was changed from

==========
Register a default instance of FeatureList for content browser.

Normally, this is done in chrome_browser_main.cc in Chrome, but
given that we can't expect the embedder to necessarily initialize
the FeatureList, provide a default one if the embedder didn't.

This fixes an error (encountered by CLs starting to use FeatureList)
when running Content Shell, which previously didn't initialize the
FeatureList.

BUG=548289
==========

to

==========
Register a default instance of FeatureList for content browser.

Normally, this is done in chrome_browser_main.cc in Chrome, but
given that we can't expect the embedder to necessarily initialize
the FeatureList, provide a default one if the embedder didn't.

This fixes an error when running Content Shell encountered by CLs
starting to use FeatureList, which didn't initialize the FeatureList
prior to this CL.

BUG=548289
==========

[Alexei Svitkine (slow), 2015-10-28 20:12:27]

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

[jam, 2015-10-29 16:18:02]

in general, this isn't a pattern that we use for content/chrome. we prefer to
have content initialize everything that it needs, without checking if an
embedder already did so before or not (i.e. otherwise code can move in either
modules and can change which module initializes it).

what about always creating it in content, and having chrome get the global
FeatureList and initialize it later with variation service data?

[Alexei Svitkine (slow), 2015-10-29 16:26:39]

On 2015/10/29 16:18:02, jam wrote:
> in general, this isn't a pattern that we use for content/chrome. we prefer to
> have content initialize everything that it needs, without checking if an
> embedder already did so before or not (i.e. otherwise code can move in either
> modules and can change which module initializes it).
> 
> what about always creating it in content, and having chrome get the global
> FeatureList and initialize it later with variation service data?

The way the API is designed, is it's either in "initialized" or "not
initialized" state. There's no in-between (i.e. no "registered FeatureList but
doesn't have variations yet"), as having one would be error-prone (i.e. code
trying to use the API too early).

It's tricky to support the flow you suggest, since the VariationsService
associations need to happen on a FeatureList after it's been created but before
it's been SetInstance'd.

The other complication is the FeatureList gets associated with variations via
VariationsService::CreateTrialsFromSeed(), which also is responsible for
creating FieldTrial objects. So it's not very easy to move that to a different
place (e.g. in a client interface callback) without changing when field trials
are created during start up, which I'd rather not do.

[Alexei Svitkine (slow), 2015-10-30 16:46:31]

Would appreciate if we could find a solution to this today. This is blocking a
CL that's making a change to use the FeatureList API.

WDYT with regard to my earlier comment? It doesn't seem very feasible to do it
the way you suggest for the reason I mentioned there.

[Alexei Svitkine (slow), 2015-11-02 16:44:51]

I now moved the logic to inside FeatureList, so it can be considered an
implementation detail of FeatureList of how that logic is done.

I think this makes a little bit nicer, since the content code doesn't need to
know about the internals of FeatureList.

(It still keeps the same flow however, because changing this is not very
feasible per my previous replies.)

PTAL when you have a chance. Thanks!

[jam, 2015-11-02 21:36:35]

On 2015/11/02 16:44:51, Alexei Svitkine (slow) wrote:
> I now moved the logic to inside FeatureList, so it can be considered an
> implementation detail of FeatureList of how that logic is done.
> 
> I think this makes a little bit nicer, since the content code doesn't need to
> know about the internals of FeatureList.
> 
> (It still keeps the same flow however, because changing this is not very
> feasible per my previous replies.)
> 
> PTAL when you have a chance. Thanks!

ok, lgtm for patchset 1 or 2, up to you

[Alexei Svitkine (slow), 2015-11-02 21:39:19]

Thanks! I prefer patchset 2 since it encapsulates the logic in FeatureList code
- thanks for making me think about it and making it slightly cleaner.

[Alexei Svitkine (slow), 2015-11-02 21:39:22]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2015-11-02 21:40:53]

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

[commit-bot: I haz the power, 2015-11-02 21:52:17]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-11-02 21:53:08]

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