Features API is not supported for content shell and webview |
||||
Issue descriptionI realized that Features API is not supported on content shell, i.e., when I set some features via command line for content shell, it does not work. build/android/adb_content_shell_command_line --enable-features=ImeThread
,
Mar 18 2016
Uploaded https://codereview.chromium.org/1812883003 for review.
,
Mar 23 2016
I noticed that features API does not work on webview, either. Also, features APIs are not being applied to lots of tests, such as content_browsertests. I initially thought that this should be a simple fix, and I wanted to fix it asap, so I digged it a bit. I ended up finding it fairly complicated. I tried several approaches, and here are the results so far. I'll take approach #2 for now because it's improving the situation, although you cannot test all combination of the features locally. (as set by fieldtrial_config*.json) Approach #1. Keep test_suite.cc to create a dummy instance, but instantiate a real instance in BrowserMainParts::PreCreateThread(). (I believe that this is in-line with what's suggested in code review.) The problem with this approach is that you cannot modify feature values in SetUp(), because PreCreateThread() is called after SetUp() is called. Currently, AsyncRevalidationManagerBrowserTest::SetUp() is the only function that does this. So basically, with this approach what happens is: 1-a) When not running a test: PreCreateThread() creates an instance once. 1-b) When running a normal test: TestSuite creates a dummy instance, and then PreCreateThread() overrides it again with a real instance. 1-c) When running a test that overrides feature in SetUp(): TestSuite creates a dummy instance, and Test::SetUp() overrides it with a new instance when needed, and then PreCreateThread() overrides it again with a real instance. If you cannot override feature value in SetUp(), things can easily get ugly. Example implementation: https://codereview.chromium.org/1821603002/#ps370001 (AsyncRevalidationManagerBrowserTest should be modified to avoid overriding feature in SetUp) Approach #2. Run browsertests and other C++ tests with dummy instance. 1-a) When not running a test: PreCreateThread() creates an instance once. 1-b) When running a normal test: TestSuite creates a dummy instance, and then PreCreateThread() does NOT override it. Features do not apply to C++ tests. 1-c) When running a test that overrides feature in SetUp(): TestSuite creates a dummy instance, and Test::SetUp() overrides it with a new instance when needed. PreCreateThread() does NOT override it. https://codereview.chromium.org/1821603002/#ps330001 Approach #3. Remove dummy instantiation from TestSuite, and add dummy instantiation or real instantiation to all that apply. But in order to support feature overriding in SetUp(), we sometimes need to avoid real instantiation. 1-a) When not running a test: PreCreateThread() creates an instance once. 1-b) When running a normal test: TestSuite creates either a dummy instance (unit test), or a real instance (browser tests and others) 1-c) When running a test that overrides feature in SetUp(): In the case of browser test, Test::SetUp() creates its own new instance, and PreCreateThread() does not override it. Example implementation: https://codereview.chromium.org/1821603002/#ps350001
,
Mar 31 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5b9da199ca3d2cc6e4bc25861df705f6e3151c04 commit 5b9da199ca3d2cc6e4bc25861df705f6e3151c04 Author: changwan <changwan@chromium.org> Date: Thu Mar 31 07:36:19 2016 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 Review URL: https://codereview.chromium.org/1812883003 Cr-Commit-Position: refs/heads/master@{#384212} [modify] https://crrev.com/5b9da199ca3d2cc6e4bc25861df705f6e3151c04/base/feature_list.cc [modify] https://crrev.com/5b9da199ca3d2cc6e4bc25861df705f6e3151c04/base/feature_list.h [modify] https://crrev.com/5b9da199ca3d2cc6e4bc25861df705f6e3151c04/base/feature_list_unittest.cc [modify] https://crrev.com/5b9da199ca3d2cc6e4bc25861df705f6e3151c04/content/browser/browser_main_loop.cc [modify] https://crrev.com/5b9da199ca3d2cc6e4bc25861df705f6e3151c04/content/browser/loader/resource_dispatcher_host_unittest.cc
,
Mar 31 2016
,
Mar 31 2016
Just to clarify, the final solution was based on approach 2 from comment #3, but it also fixed command-line features to support content browser tests and other applicable tests. |
||||
►
Sign in to add a comment |
||||
Comment 1 by changwan@chromium.org
, Mar 18 2016