Enable features API for content shell and webview

Enable features API for content shell and webview

When I run the following:

build/android/adb_content_shell_command_line --enable-features=ImeThread

Then call to base::FeatureList::IsEnabled(features::kImeThread)
always returns false.

This happens because we are not initializing feature list for content shell.

With this CL, BrowserMainLoop::PreCreateThreads() now initializes
features from
commandline.

BUG=596021
Committed: https://crrev.com/5b9da199ca3d2cc6e4bc25861df705f6e3151c04
Cr-Commit-Position: refs/heads/master@{#384212}

[Changwan Ryu, 2016-03-18 10:46:24]

changwan@chromium.org changed reviewers:
+ sievers@chromium.org

[Changwan Ryu, 2016-03-18 10:46:24]

PTAL

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

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

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

Thanks for working on this!

https://codereview.chromium.org/1812883003/diff/1/content/shell/browser/shell...
File content/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/1812883003/diff/1/content/shell/browser/shell...
content/shell/browser/shell_browser_main_parts.cc:138: const base::CommandLine*
command_line =
Indent is wrong.

https://codereview.chromium.org/1812883003/diff/1/content/shell/browser/shell...
content/shell/browser/shell_browser_main_parts.cc:145:
base::FeatureList::SetInstance(std::move(feature_list));
So this fails because an instance was already registered.

See this CL for how I initially made content shell initialize its FeatureList:

https://codereview.chromium.org/1409933007

The issue is that setting up the instance must only be done once. Right now,
this is done in Chrome code or in FeatureList::InitializeInstance() by content -
which is a no-op if Chrome code already initialized it.

You probably want to expand FeatureList::InitializeInstance() implementation -
with maybe passing the two flags to it.

[Changwan Ryu, 2016-03-23 09:47:36]

https://codereview.chromium.org/1812883003/diff/1/content/shell/browser/shell...
File content/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/1812883003/diff/1/content/shell/browser/shell...
content/shell/browser/shell_browser_main_parts.cc:138: const base::CommandLine*
command_line =
On 2016/03/18 15:03:42, Alexei Svitkine (very slow) wrote:
> Indent is wrong.

Done.

https://codereview.chromium.org/1812883003/diff/1/content/shell/browser/shell...
content/shell/browser/shell_browser_main_parts.cc:145:
base::FeatureList::SetInstance(std::move(feature_list));
On 2016/03/18 15:03:42, asvitkine (no revs until Thu) wrote:
> So this fails because an instance was already registered.
> 
> See this CL for how I initially made content shell initialize its FeatureList:
> 
> https://codereview.chromium.org/1409933007
> 
> The issue is that setting up the instance must only be done once. Right now,
> this is done in Chrome code or in FeatureList::InitializeInstance() by content
-
> which is a no-op if Chrome code already initialized it.
> 
> You probably want to expand FeatureList::InitializeInstance() implementation -
> with maybe passing the two flags to it.

Please check crbug.com/596021 for the different approaches I took.
I could not find a perfect solution for this, but instead chose the one that
improves the situation. WDYT?

[Alexei Svitkine (slow), 2016-03-24 17:16:37]

Thanks for your detailed investigation. See below for further discussion.

https://codereview.chromium.org/1812883003/diff/60001/content/shell/browser/s...
File content/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/1812883003/diff/60001/content/shell/browser/s...
content/shell/browser/shell_browser_main_parts.cc:149:
base::FeatureList::SetInstance(std::move(feature_list));
Instead of requiring every BrowserMainParts implementation to do this, can this
be done in a place where it's only done once?

I think the right place is the current call to
base::FeatureList::InitializeInstance() from browser_main_loop.cc - is there a
reason we simply can't add the enable_features & disable_features params to that
function?

[Changwan Ryu, 2016-03-28 06:58:53]

Description was changed from

==========
Enable features API for content shell

When I run the following:

build/android/adb_content_shell_command_line --enable-features=ImeThread

Then call to base::FeatureList::IsEnabled(features::kImeThread)
always returns false.

This happens because we are not initializing feature list for content shell.

BrowserMainLoop::PreCreateThreads() will call
BrowserMainParts::PreCreateThreads() so this seems to be the right place
to add such initialization.

BUG=596021
==========

to

==========
Enable features API for content shell

When I run the following:

build/android/adb_content_shell_command_line --enable-features=ImeThread

Then call to base::FeatureList::IsEnabled(features::kImeThread)
always returns false.

This happens because we are not initializing feature list for content shell.

BrowserMainLoop::PreCreateThreads() now initializes
features from
commandline.
Note that features still do not apply to some tests.

BUG=596021
==========

[Changwan Ryu, 2016-03-28 07:09:27]

PTAL

https://codereview.chromium.org/1812883003/diff/60001/content/shell/browser/s...
File content/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/1812883003/diff/60001/content/shell/browser/s...
content/shell/browser/shell_browser_main_parts.cc:149:
base::FeatureList::SetInstance(std::move(feature_list));
On 2016/03/24 17:16:37, Alexei Svitkine (OOO 25-27th) wrote:
> Instead of requiring every BrowserMainParts implementation to do this, can
this
> be done in a place where it's only done once?
> 
> I think the right place is the current call to
> base::FeatureList::InitializeInstance() from browser_main_loop.cc - is there a
> reason we simply can't add the enable_features & disable_features params to
that
> function?

Makes perfect sense that this should be done inside browser_main_loop.cc, so
I've moved it there.

Extending InitializeInstance() with the two parameters seemed a bit unclear to
me as to whether it would override the existing feature list, or newly
instantiate one, and in order to avoid string searches in the case of chrome and
tests, I left the code as is. Please let me know if you have a better idea.

[Changwan Ryu, 2016-03-29 09:40:13]

On 2016/03/28 07:09:27, Changwan Ryu wrote:
> PTAL
> 
>
https://codereview.chromium.org/1812883003/diff/60001/content/shell/browser/s...
> File content/shell/browser/shell_browser_main_parts.cc (right):
> 
>
https://codereview.chromium.org/1812883003/diff/60001/content/shell/browser/s...
> content/shell/browser/shell_browser_main_parts.cc:149:
> base::FeatureList::SetInstance(std::move(feature_list));
> On 2016/03/24 17:16:37, Alexei Svitkine (OOO 25-27th) wrote:
> > Instead of requiring every BrowserMainParts implementation to do this, can
> this
> > be done in a place where it's only done once?
> > 
> > I think the right place is the current call to
> > base::FeatureList::InitializeInstance() from browser_main_loop.cc - is there
a
> > reason we simply can't add the enable_features & disable_features params to
> that
> > function?
> 
> Makes perfect sense that this should be done inside browser_main_loop.cc, so
> I've moved it there.
> 
> Extending InitializeInstance() with the two parameters seemed a bit unclear to
> me as to whether it would override the existing feature list, or newly
> instantiate one, and in order to avoid string searches in the case of chrome
and
> tests, I left the code as is. Please let me know if you have a better idea.

ping?

[Alexei Svitkine (slow), 2016-03-29 15:08:03]

https://codereview.chromium.org/1812883003/diff/60001/content/shell/browser/s...
File content/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/1812883003/diff/60001/content/shell/browser/s...
content/shell/browser/shell_browser_main_parts.cc:149:
base::FeatureList::SetInstance(std::move(feature_list));
On 2016/03/28 07:09:27, Changwan Ryu wrote:
> On 2016/03/24 17:16:37, Alexei Svitkine (OOO 25-27th) wrote:
> > Instead of requiring every BrowserMainParts implementation to do this, can
> this
> > be done in a place where it's only done once?
> > 
> > I think the right place is the current call to
> > base::FeatureList::InitializeInstance() from browser_main_loop.cc - is there
a
> > reason we simply can't add the enable_features & disable_features params to
> that
> > function?
> 
> Makes perfect sense that this should be done inside browser_main_loop.cc, so
> I've moved it there.
> 
> Extending InitializeInstance() with the two parameters seemed a bit unclear to
> me as to whether it would override the existing feature list, or newly
> instantiate one, and in order to avoid string searches in the case of chrome
and
> tests, I left the code as is. Please let me know if you have a better idea.

I did not mean that the two params should override an existing feature list if
it exists - only register it with those params if there isn't one already. I
understand doing so will be a little inefficient since you have to look up the
two command line params before the instance check is done, but I think that's a
trivial performance cost especially since this is only done once on start up.

[Changwan Ryu, 2016-03-29 19:24:27]

Description was changed from

==========
Enable features API for content shell

When I run the following:

build/android/adb_content_shell_command_line --enable-features=ImeThread

Then call to base::FeatureList::IsEnabled(features::kImeThread)
always returns false.

This happens because we are not initializing feature list for content shell.

BrowserMainLoop::PreCreateThreads() now initializes
features from
commandline.
Note that features still do not apply to some tests.

BUG=596021
==========

to

==========
Enable features API for content shell and webview

When I run the following:

build/android/adb_content_shell_command_line --enable-features=ImeThread

Then call to base::FeatureList::IsEnabled(features::kImeThread)
always returns false.

This happens because we are not initializing feature list for content shell.

With this CL, BrowserMainLoop::PreCreateThreads() now initializes
features from
commandline.

BUG=596021
==========

[Changwan Ryu, 2016-03-29 19:24:47]

On 2016/03/29 15:08:03, Alexei Svitkine wrote:
>
https://codereview.chromium.org/1812883003/diff/60001/content/shell/browser/s...
> File content/shell/browser/shell_browser_main_parts.cc (right):
> 
>
https://codereview.chromium.org/1812883003/diff/60001/content/shell/browser/s...
> content/shell/browser/shell_browser_main_parts.cc:149:
> base::FeatureList::SetInstance(std::move(feature_list));
> On 2016/03/28 07:09:27, Changwan Ryu wrote:
> > On 2016/03/24 17:16:37, Alexei Svitkine (OOO 25-27th) wrote:
> > > Instead of requiring every BrowserMainParts implementation to do this, can
> > this
> > > be done in a place where it's only done once?
> > > 
> > > I think the right place is the current call to
> > > base::FeatureList::InitializeInstance() from browser_main_loop.cc - is
there
> a
> > > reason we simply can't add the enable_features & disable_features params
to
> > that
> > > function?
> > 
> > Makes perfect sense that this should be done inside browser_main_loop.cc, so
> > I've moved it there.
> > 
> > Extending InitializeInstance() with the two parameters seemed a bit unclear
to
> > me as to whether it would override the existing feature list, or newly
> > instantiate one, and in order to avoid string searches in the case of chrome
> and
> > tests, I left the code as is. Please let me know if you have a better idea.
> 
> I did not mean that the two params should override an existing feature list if
> it exists - only register it with those params if there isn't one already. I
> understand doing so will be a little inefficient since you have to look up the
> two command line params before the instance check is done, but I think that's
a
> trivial performance cost especially since this is only done once on start up.

PTAL

[Alexei Svitkine (slow), 2016-03-30 16:44:32]

https://codereview.chromium.org/1812883003/diff/100001/base/feature_list.cc
File base/feature_list.cc (right):

https://codereview.chromium.org/1812883003/diff/100001/base/feature_list.cc#n...
base/feature_list.cc:149: if (g_instance->initialized_from_command_line_) {
Are you sure this bit of logic is necessary?

My understanding is that the other places that create an instance do initialize
it from the command line, so if it exists, this would already be true.

[Alexei Svitkine (slow), 2016-03-30 17:12:25]

https://codereview.chromium.org/1812883003/diff/100001/base/feature_list.cc
File base/feature_list.cc (right):

https://codereview.chromium.org/1812883003/diff/100001/base/feature_list.cc#n...
base/feature_list.cc:149: if (g_instance->initialized_from_command_line_) {
On 2016/03/30 16:44:32, Alexei Svitkine wrote:
> Are you sure this bit of logic is necessary?
> 
> My understanding is that the other places that create an instance do
initialize
> it from the command line, so if it exists, this would already be true.

We chatted more offline. This check is needed to support flags for content
browser tests - since in that case the instance is initialized by test_suite.cc
which is in base/ and can't use the command flags.

Can you add a comment mentioning base/test/test_suite.cc as an example where
this could be false?

https://codereview.chromium.org/1812883003/diff/100001/base/feature_list.cc#n...
base/feature_list.cc:151: } else {
Nit: No need for else if there's a return above.

[Changwan Ryu, 2016-03-30 17:53:31]

https://codereview.chromium.org/1812883003/diff/100001/base/feature_list.cc
File base/feature_list.cc (right):

https://codereview.chromium.org/1812883003/diff/100001/base/feature_list.cc#n...
base/feature_list.cc:149: if (g_instance->initialized_from_command_line_) {
On 2016/03/30 17:12:25, Alexei Svitkine wrote:
> On 2016/03/30 16:44:32, Alexei Svitkine wrote:
> > Are you sure this bit of logic is necessary?
> > 
> > My understanding is that the other places that create an instance do
> initialize
> > it from the command line, so if it exists, this would already be true.
> 
> We chatted more offline. This check is needed to support flags for content
> browser tests - since in that case the instance is initialized by
test_suite.cc
> which is in base/ and can't use the command flags.
> 
> Can you add a comment mentioning base/test/test_suite.cc as an example where
> this could be false?

Added some comment.

https://codereview.chromium.org/1812883003/diff/100001/base/feature_list.cc#n...
base/feature_list.cc:151: } else {
On 2016/03/30 17:12:25, Alexei Svitkine wrote:
> Nit: No need for else if there's a return above.

Done.

[Alexei Svitkine (slow), 2016-03-30 18:05:04]

lgtm

[Changwan Ryu, 2016-03-30 18:06:21]

On 2016/03/30 18:05:04, Alexei Svitkine wrote:
> lgtm

sievers@, could you take a look at content/ ?

[no sievers, 2016-03-30 18:36:03]

lgtm

https://codereview.chromium.org/1812883003/diff/120001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1812883003/diff/120001/content/browser/browse...
content/browser/browser_main_loop.cc:698: // Initialize from command line flags
only if we haven't yet.
nit: the 'only if we haven't yet' comment does not match the code. Did you mean
it's safe to call this repeatedly and it does nothing the second time?

[Changwan Ryu, 2016-03-31 00:31:46]

Thanks for all the reviews.

https://codereview.chromium.org/1812883003/diff/120001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1812883003/diff/120001/content/browser/browse...
content/browser/browser_main_loop.cc:698: // Initialize from command line flags
only if we haven't yet.
On 2016/03/30 18:36:03, sievers wrote:
> nit: the 'only if we haven't yet' comment does not match the code. Did you
mean
> it's safe to call this repeatedly and it does nothing the second time?

Updated the comment.

[Changwan Ryu, 2016-03-31 00:31:57]

The CQ bit was checked by changwan@chromium.org

[Changwan Ryu, 2016-03-31 00:31:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1812883003/#ps140001
(title: "updated comment in BrowserMainLoop")

[commit-bot: I haz the power, 2016-03-31 00:32:57]

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

[commit-bot: I haz the power, 2016-03-31 00:50:32]

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

[commit-bot: I haz the power, 2016-03-31 00:50:34]

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

[Changwan Ryu, 2016-03-31 01:07:33]

The CQ bit was checked by changwan@chromium.org

[commit-bot: I haz the power, 2016-03-31 01:07:50]

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

[Changwan Ryu, 2016-03-31 07:07:32]

The CQ bit was unchecked by changwan@chromium.org

[Changwan Ryu, 2016-03-31 07:07:40]

The CQ bit was checked by changwan@chromium.org

[commit-bot: I haz the power, 2016-03-31 07:07:55]

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

[commit-bot: I haz the power, 2016-03-31 07:16:49]

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

[commit-bot: I haz the power, 2016-03-31 07:16:50]

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

[Changwan Ryu, 2016-03-31 07:17:25]

The CQ bit was checked by changwan@chromium.org

[commit-bot: I haz the power, 2016-03-31 07:17:46]

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

[commit-bot: I haz the power, 2016-03-31 07:20:23]

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

[commit-bot: I haz the power, 2016-03-31 07:20:24]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Changwan Ryu, 2016-03-31 07:21:19]

The CQ bit was checked by changwan@chromium.org

[commit-bot: I haz the power, 2016-03-31 07:21:32]

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

[commit-bot: I haz the power, 2016-03-31 07:36:28]

Description was changed from

==========
Enable features API for content shell and webview

When I run the following:

build/android/adb_content_shell_command_line --enable-features=ImeThread

Then call to base::FeatureList::IsEnabled(features::kImeThread)
always returns false.

This happens because we are not initializing feature list for content shell.

With this CL, BrowserMainLoop::PreCreateThreads() now initializes
features from
commandline.

BUG=596021
==========

to

==========
Enable features API for content shell and webview

When I run the following:

build/android/adb_content_shell_command_line --enable-features=ImeThread

Then call to base::FeatureList::IsEnabled(features::kImeThread)
always returns false.

This happens because we are not initializing feature list for content shell.

With this CL, BrowserMainLoop::PreCreateThreads() now initializes
features from
commandline.

BUG=596021
==========

[commit-bot: I haz the power, 2016-03-31 07:36:29]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-03-31 07:38:07]

Description was changed from

==========
Enable features API for content shell and webview

When I run the following:

build/android/adb_content_shell_command_line --enable-features=ImeThread

Then call to base::FeatureList::IsEnabled(features::kImeThread)
always returns false.

This happens because we are not initializing feature list for content shell.

With this CL, BrowserMainLoop::PreCreateThreads() now initializes
features from
commandline.

BUG=596021
==========

to

==========
Enable features API for content shell and webview

When I run the following:

build/android/adb_content_shell_command_line --enable-features=ImeThread

Then call to base::FeatureList::IsEnabled(features::kImeThread)
always returns false.

This happens because we are not initializing feature list for content shell.

With this CL, BrowserMainLoop::PreCreateThreads() now initializes
features from
commandline.

BUG=596021

Committed: https://crrev.com/5b9da199ca3d2cc6e4bc25861df705f6e3151c04
Cr-Commit-Position: refs/heads/master@{#384212}
==========

[commit-bot: I haz the power, 2016-03-31 07:38:09]

Patchset 8 (id:??) landed as
https://crrev.com/5b9da199ca3d2cc6e4bc25861df705f6e3151c04
Cr-Commit-Position: refs/heads/master@{#384212}