Java version of FeatureList::IsEnabled |
|||||||
Issue descriptionI'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?
,
Oct 9
,
Oct 10
,
Oct 10
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.
,
Oct 11
Issue 801009 has been merged into this issue.
,
Oct 11
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
,
Oct 19
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.
,
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
,
Oct 19
Keeping this open to see where it makes sense to share code, and if we can add the @EnableFeatures annotation for WebView.
,
Nov 5
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
,
Dec 6
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 |
|||||||
Comment 1 by ntfschr@chromium.org
, Oct 9