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

Issue 892834 link

Starred by 2 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Feature

Blocking:
issue 748687



Sign in to add a comment

Java version of FeatureList::IsEnabled

Project Member Reported by ntfschr@chromium.org, Oct 5

Issue description

I'd like to check if a Feature is enabled, but I see there's no base/ layer API to do so from Java (only from native). I'm aware of ChromeFeatureList.isEnabled(), but I'd like to check outside of Chrome layer (I'm specifically working in the android_webview/ layer).

Can we add a general Java FeatureList#isEnabled() under base/? Or, should I implement AndroidWebViewFeatureList based on ChromeFeatureList?
 
Blocking: 748687
Cc: vakh@chromium.org
Owner: ----
Status: Available (was: Assigned)
I would suggest going with AndroidWebViewFeatureList to match Chrome/.

Or perhaps we can make a component for it? e.g. components/android_features/ that could be used by both Chrome and Webview?

The problem with having it with base/ is it needs to depend on the actual places where those features are defined. And they can be defined throughout the codebase - from components, to chrome/ to anywhere. We can't make base/ depend on it.

And the system needs to be refer to the base::Feature structs because they encode the default state which is needed.

Another option is to have some kind of code-gen that generates Java-language parallels to base::Feature C++ structs (e.g. similar to enums which I think exists? - i.e. something that generates both C++ struct and Java equivalent). Then, Java side could use the default state that's mirrored in Java along with calling into C++ to get the overrides for it. But this kind of system sounds pretty involved.

The other option is to just expose JNI for specific features you want to test.
Issue 801009 has been merged into this issue.
I see content/ layer also does the same thing [1].

Could we compromise by keeping the lists in the relevant layer, and move the logic (e.g., FindFeatureExposedToJava()) into base/?

[1] https://cs.chromium.org/chromium/src/content/browser/android/content_feature_list.cc?l=29&rcl=5b2786cb7ab7b3956f48876c1d4b34cc4e971dfb
Owner: ntfschr@chromium.org
Status: Assigned (was: Available)
I'll try to take this up to unblock issue 748687. I'll duplicate the chrome code in AW for now, but I'll try to follow-up and move things to base/ where it makes sense to do so.
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1699c0728ebfa2bd8faec565d4a46e86f7932150

commit 1699c0728ebfa2bd8faec565d4a46e86f7932150
Author: Nate Fischer <ntfschr@chromium.org>
Date: Fri Oct 19 22:00:17 2018

AW: add Feature for connectionless Safe Browsing

We're in the process of moving from the deprecated connection-based GMS
SafetyNet APIs to the connectionless APIs. Unfortunately, these APIs
behave quite differently, so I'm creating a Feature to make the
transition cautiously.

Feature-checking logic is plumbed through AwFeatureList to Java so
it can be checked downstream (unfortunately, there's no base/ layer API
to check features by name). AwFeatureList is based on
ContentFeatureList and ChromeFeatureList (and we may be able to
share some logic in base layer).

As this Feature is off-by-default, there is no change to behavior.

Also, see the downstream CL (http://crrev/i/694949).

Bug: 748687
Bug: 892834
Test: N/A
Change-Id: I5d6642d7bc236836f4ab2a4dbf8888f88b18e479
Reviewed-on: https://chromium-review.googlesource.com/c/1272595
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Reviewed-by: Changwan Ryu <changwan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601311}
[modify] https://crrev.com/1699c0728ebfa2bd8faec565d4a46e86f7932150/android_webview/BUILD.gn
[add] https://crrev.com/1699c0728ebfa2bd8faec565d4a46e86f7932150/android_webview/browser/aw_feature_list.cc
[add] https://crrev.com/1699c0728ebfa2bd8faec565d4a46e86f7932150/android_webview/browser/aw_feature_list.h
[add] https://crrev.com/1699c0728ebfa2bd8faec565d4a46e86f7932150/android_webview/java/src/org/chromium/android_webview/AwFeatureList.java

Keeping this open to see where it makes sense to share code, and if we can add the @EnableFeatures annotation for WebView.
Project Member

Comment 10 by bugdroid1@chromium.org, Nov 5

Labels: merge-merged-3578
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a3cfa12055e0c894770e4dc3764942d66337a327

commit a3cfa12055e0c894770e4dc3764942d66337a327
Author: Nate Fischer <ntfschr@chromium.org>
Date: Mon Nov 05 20:17:29 2018

AW: add Feature for connectionless Safe Browsing

[The feature list code in this patch is needed by https://chromium-review.googlesource.com/c/chromium/src/+/1306514
This isn't intend to cherry-pick safe browsing feature.]

We're in the process of moving from the deprecated connection-based GMS
SafetyNet APIs to the connectionless APIs. Unfortunately, these APIs
behave quite differently, so I'm creating a Feature to make the
transition cautiously.

Feature-checking logic is plumbed through AwFeatureList to Java so
it can be checked downstream (unfortunately, there's no base/ layer API
to check features by name). AwFeatureList is based on
ContentFeatureList and ChromeFeatureList (and we may be able to
share some logic in base layer).

As this Feature is off-by-default, there is no change to behavior.

Also, see the downstream CL (http://crrev/i/694949).

Bug: 748687
Bug: 892834
Test: N/A
Change-Id: I5d6642d7bc236836f4ab2a4dbf8888f88b18e479
Reviewed-on: https://chromium-review.googlesource.com/c/1272595
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Reviewed-by: Changwan Ryu <changwan@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#601311}(cherry picked from commit 1699c0728ebfa2bd8faec565d4a46e86f7932150)
Reviewed-on: https://chromium-review.googlesource.com/c/1318531
Reviewed-by: Tao Bai <michaelbai@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#518}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/a3cfa12055e0c894770e4dc3764942d66337a327/android_webview/BUILD.gn
[add] https://crrev.com/a3cfa12055e0c894770e4dc3764942d66337a327/android_webview/browser/aw_feature_list.cc
[add] https://crrev.com/a3cfa12055e0c894770e4dc3764942d66337a327/android_webview/browser/aw_feature_list.h
[add] https://crrev.com/a3cfa12055e0c894770e4dc3764942d66337a327/android_webview/java/src/org/chromium/android_webview/AwFeatureList.java

Owner: ----
Status: Available (was: Assigned)
Remaining work:

 * Try to consolidate logic for "look up a feature by name" into a single method (right now we have 3 copies of FindFeatureExposedToJava)
 * Implement EnableFeature() and DisableFeature() annotations in the base layer (This patch almost works, but I don't have time to debug this http://crrev/c/1341130)

Sign in to add a comment