New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 596021 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

Features API is not supported for content shell and webview

Project Member Reported by changwan@chromium.org, Mar 18 2016

Issue description

I 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

 
Cc: tedc...@chromium.org
Cc: -tedc...@chromium.org siev...@chromium.org
Uploaded https://codereview.chromium.org/1812883003 for review.
Summary: Features API is not supported for content shell and webview (was: Features API is not supported for content shell)
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

Project Member

Comment 4 by bugdroid1@chromium.org, 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

Labels: M-51
Status: Fixed (was: Assigned)
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