Add feature flag check for split show password setting [chromium/src : main]
Md Shahadat Hossain Shahin (Gerrit)
Md Shahadat Hossain Shahin has uploaded the change for review![]( "Open in Gerrit")
Commit message
Add feature flag check for split show password setting
Change diff
diff --git a/components/embedder_support/android/java/src/org/chromium/components/embedder_support/util/PasswordEchoSettingState.java b/components/embedder_support/android/java/src/org/chromium/components/embedder_support/util/PasswordEchoSettingState.java
index 505d914..1c5bdec 100644
--- a/components/embedder_support/android/java/src/org/chromium/components/embedder_support/util/PasswordEchoSettingState.java
+++ b/components/embedder_support/android/java/src/org/chromium/components/embedder_support/util/PasswordEchoSettingState.java
@@ -7,7 +7,9 @@
import android.content.ContentResolver;
import android.database.ContentObserver;
import android.net.Uri;
+import android.os.Build;
import android.os.Handler;
+import android.os.flagging.AconfigPackage;
import android.provider.Settings;
import androidx.annotation.VisibleForTesting;
@@ -73,8 +75,15 @@
return sSplitEnabledForTesting;
}
- // TODO(crbug.com/466343369): Implement the logic to check if the split setting feature is
- // enabled on the platform side.
+ if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.BAKLAVA) {
+ try {
+ return AconfigPackage.load("com.android.text.flags")
+ .getBooleanFlagValue("split_show_passwords_to_touch_and_physical", false);
+ } catch (Exception e) {
+ // Default to checking the legacy settings if we error checking the flag value.
+ return false;
+ }
+ }
return false;
}
Change information
- M components/embedder_support/android/java/src/org/chromium/components/embedder_support/util/PasswordEchoSettingState.java
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Md Shahadat Hossain Shahin (Gerrit)
Md Shahadat Hossain Shahin added 1 comment![]( "Open in Gerrit")
Hi Peter, could you please review before I send it to the owners?
Related details
- Peter Conn
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Conn (Gerrit)
Peter Conn added 1 comment![]( "Open in Gerrit")
return AconfigPackage.load("com.android.text.flags")This is the only usage of AconfigPackage in Chromium, and the [docs](https://developer.android.com/reference/android/os/flagging/AconfigPackage) mention:
Note: this is intended only to be used by generated code. To determine if a given flag is enabled in app code, the generated android flags should be used.
Are you sure this is the right way to do this? (I have no idea myself.) Potentially it's worth mailing Chromium / Chrome on Android development lists?
Related details
- Md Shahadat Hossain Shahin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Md Shahadat Hossain Shahin (Gerrit)
Md Shahadat Hossain Shahin added 2 comments![]( "Open in Gerrit")
Hi Richard, could you please take a look if this a valid way to check aconfig flag value from Chrome code? Thanks!
return AconfigPackage.load("com.android.text.flags")This is the only usage of AconfigPackage in Chromium, and the [docs](https://developer.android.com/reference/android/os/flagging/AconfigPackage) mention:
Note: this is intended only to be used by generated code. To determine if a given flag is enabled in app code, the generated android flags should be used.
Are you sure this is the right way to do this? (I have no idea myself.) Potentially it's worth mailing Chromium / Chrome on Android development lists?
Thanks for taking a look. I am also unsure about this. We previously had a discussion with torne@ about checking aconfig flag value from Chrome. Looping him in for comments.
Related details
- Richard (Torne) Coles
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Richard (Torne) Coles (Gerrit)
Richard (Torne) Coles added 1 comment![]( "Open in Gerrit")
return AconfigPackage.load("com.android.text.flags")Md Shahadat Hossain ShahinThis is the only usage of AconfigPackage in Chromium, and the [docs](https://developer.android.com/reference/android/os/flagging/AconfigPackage) mention:
Note: this is intended only to be used by generated code. To determine if a given flag is enabled in app code, the generated android flags should be used.
Are you sure this is the right way to do this? (I have no idea myself.) Potentially it's worth mailing Chromium / Chrome on Android development lists?
Thanks for taking a look. I am also unsure about this. We previously had a discussion with torne@ about checking aconfig flag value from Chrome. Looping him in for comments.
We have the unreleased Android SDK including the generated flags library referred to by the docs here available in the downstream //clank repository, and we have a mechanism set up to allow this to be used from upstream via an abstraction that is a no-op in public builds, and that's what you should be using here.
See https://source.chromium.org/chromium/chromium/src/+/main:base/android/java/src/org/chromium/base/AconfigFlaggedApiDelegate.java for the public interface defined upstream, and see the `AconfigFlaggedApiDelegateImpl` class in `//clank` for the actual implementations which check flags and call new APIs.
You shouldn't *just* be checking the flag here - you should also be using the actual framework APIs/constants to access the setting values you need, instead of copying their definitions into Chromium. i.e. you should just use the `android.text.ShowSecretsSetting` APIs in the implementation downstream.
Related details
- Md Shahadat Hossain Shahin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Md Shahadat Hossain Shahin (Gerrit)
Md Shahadat Hossain Shahin added 2 comments![]( "Open in Gerrit")
Thanks for taking a look. I have added few follow up questions. Please take another look.
return AconfigPackage.load("com.android.text.flags")Md Shahadat Hossain ShahinThis is the only usage of AconfigPackage in Chromium, and the [docs](https://developer.android.com/reference/android/os/flagging/AconfigPackage) mention:
Note: this is intended only to be used by generated code. To determine if a given flag is enabled in app code, the generated android flags should be used.
Are you sure this is the right way to do this? (I have no idea myself.) Potentially it's worth mailing Chromium / Chrome on Android development lists?
Richard (Torne) ColesThanks for taking a look. I am also unsure about this. We previously had a discussion with torne@ about checking aconfig flag value from Chrome. Looping him in for comments.
Md Shahadat Hossain ShahinWe have the unreleased Android SDK including the generated flags library referred to by the docs here available in the downstream //clank repository, and we have a mechanism set up to allow this to be used from upstream via an abstraction that is a no-op in public builds, and that's what you should be using here.
See https://source.chromium.org/chromium/chromium/src/+/main:base/android/java/src/org/chromium/base/AconfigFlaggedApiDelegate.java for the public interface defined upstream, and see the `AconfigFlaggedApiDelegateImpl` class in `//clank` for the actual implementations which check flags and call new APIs.
You shouldn't *just* be checking the flag here - you should also be using the actual framework APIs/constants to access the setting values you need, instead of copying their definitions into Chromium. i.e. you should just use the `android.text.ShowSecretsSetting` APIs in the implementation downstream.
Thanks for the pointers!
Few follow up questions:
The CL that exported the aconfig flag was merged yesterday. Do you know how long it might take for the flag to be available in the unreleased sdk in downstream?
To add the flag check like the way you mentioned, we need to write two CLs, one in the public chromium and one in the interal //clank. If we want to make the feature available by M145, is it a must to land both CLs by the M145 branch date or only merging the public CL by the branch date is sufficient? I am asking as I am not sure how long it might take for the flag to be available in downstream.
Related details
- Richard (Torne) Coles
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Richard (Torne) Coles (Gerrit)
Richard (Torne) Coles added 1 comment![]( "Open in Gerrit")
return AconfigPackage.load("com.android.text.flags")Md Shahadat Hossain ShahinThis is the only usage of AconfigPackage in Chromium, and the [docs](https://developer.android.com/reference/android/os/flagging/AconfigPackage) mention:
Note: this is intended only to be used by generated code. To determine if a given flag is enabled in app code, the generated android flags should be used.
Are you sure this is the right way to do this? (I have no idea myself.) Potentially it's worth mailing Chromium / Chrome on Android development lists?
Richard (Torne) ColesThanks for taking a look. I am also unsure about this. We previously had a discussion with torne@ about checking aconfig flag value from Chrome. Looping him in for comments.
Md Shahadat Hossain ShahinWe have the unreleased Android SDK including the generated flags library referred to by the docs here available in the downstream //clank repository, and we have a mechanism set up to allow this to be used from upstream via an abstraction that is a no-op in public builds, and that's what you should be using here.
See https://source.chromium.org/chromium/chromium/src/+/main:base/android/java/src/org/chromium/base/AconfigFlaggedApiDelegate.java for the public interface defined upstream, and see the `AconfigFlaggedApiDelegateImpl` class in `//clank` for the actual implementations which check flags and call new APIs.
You shouldn't *just* be checking the flag here - you should also be using the actual framework APIs/constants to access the setting values you need, instead of copying their definitions into Chromium. i.e. you should just use the `android.text.ShowSecretsSetting` APIs in the implementation downstream.
Thanks for the pointers!
Few follow up questions:
The CL that exported the aconfig flag was merged yesterday. Do you know how long it might take for the flag to be available in the unreleased sdk in downstream?
To add the flag check like the way you mentioned, we need to write two CLs, one in the public chromium and one in the interal //clank. If we want to make the feature available by M145, is it a must to land both CLs by the M145 branch date or only merging the public CL by the branch date is sufficient? I am asking as I am not sure how long it might take for the flag to be available in downstream.
The SDK is autorolled weekly but the bots can be triggered manually any time - I just triggered the first step of the process and we should be able to roll to a new enough version in a couple of hours.
Both CLs would need to land before the M145 branch point, or be cherrypicked.
Related details
- Md Shahadat Hossain Shahin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Richard (Torne) Coles (Gerrit)
Richard (Torne) Coles added 1 comment![]( "Open in Gerrit")
return AconfigPackage.load("com.android.text.flags")Md Shahadat Hossain ShahinThis is the only usage of AconfigPackage in Chromium, and the [docs](https://developer.android.com/reference/android/os/flagging/AconfigPackage) mention:
Note: this is intended only to be used by generated code. To determine if a given flag is enabled in app code, the generated android flags should be used.
Are you sure this is the right way to do this? (I have no idea myself.) Potentially it's worth mailing Chromium / Chrome on Android development lists?
Richard (Torne) ColesThanks for taking a look. I am also unsure about this. We previously had a discussion with torne@ about checking aconfig flag value from Chrome. Looping him in for comments.
Md Shahadat Hossain ShahinWe have the unreleased Android SDK including the generated flags library referred to by the docs here available in the downstream //clank repository, and we have a mechanism set up to allow this to be used from upstream via an abstraction that is a no-op in public builds, and that's what you should be using here.
See https://source.chromium.org/chromium/chromium/src/+/main:base/android/java/src/org/chromium/base/AconfigFlaggedApiDelegate.java for the public interface defined upstream, and see the `AconfigFlaggedApiDelegateImpl` class in `//clank` for the actual implementations which check flags and call new APIs.
You shouldn't *just* be checking the flag here - you should also be using the actual framework APIs/constants to access the setting values you need, instead of copying their definitions into Chromium. i.e. you should just use the `android.text.ShowSecretsSetting` APIs in the implementation downstream.
Richard (Torne) ColesThanks for the pointers!
Few follow up questions:
The CL that exported the aconfig flag was merged yesterday. Do you know how long it might take for the flag to be available in the unreleased sdk in downstream?
To add the flag check like the way you mentioned, we need to write two CLs, one in the public chromium and one in the interal //clank. If we want to make the feature available by M145, is it a must to land both CLs by the M145 branch date or only merging the public CL by the branch date is sufficient? I am asking as I am not sure how long it might take for the flag to be available in downstream.
The SDK is autorolled weekly but the bots can be triggered manually any time - I just triggered the first step of the process and we should be able to roll to a new enough version in a couple of hours.
Both CLs would need to land before the M145 branch point, or be cherrypicked.
SDK version 14689923 which should contain all the changes related to this API is currently going through the autoroller: https://chrome-internal-review.googlesource.com/c/clank/internal/apps/+/8913355
So once that gets through the CQ you should be able to implement this using the real API downstream.
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Md Shahadat Hossain Shahin (Gerrit)
Md Shahadat Hossain Shahin added 1 comment![]( "Open in Gerrit")
return AconfigPackage.load("com.android.text.flags")Md Shahadat Hossain ShahinThis is the only usage of AconfigPackage in Chromium, and the [docs](https://developer.android.com/reference/android/os/flagging/AconfigPackage) mention:
Note: this is intended only to be used by generated code. To determine if a given flag is enabled in app code, the generated android flags should be used.
Are you sure this is the right way to do this? (I have no idea myself.) Potentially it's worth mailing Chromium / Chrome on Android development lists?
Richard (Torne) ColesThanks for taking a look. I am also unsure about this. We previously had a discussion with torne@ about checking aconfig flag value from Chrome. Looping him in for comments.
Md Shahadat Hossain ShahinWe have the unreleased Android SDK including the generated flags library referred to by the docs here available in the downstream //clank repository, and we have a mechanism set up to allow this to be used from upstream via an abstraction that is a no-op in public builds, and that's what you should be using here.
See https://source.chromium.org/chromium/chromium/src/+/main:base/android/java/src/org/chromium/base/AconfigFlaggedApiDelegate.java for the public interface defined upstream, and see the `AconfigFlaggedApiDelegateImpl` class in `//clank` for the actual implementations which check flags and call new APIs.
You shouldn't *just* be checking the flag here - you should also be using the actual framework APIs/constants to access the setting values you need, instead of copying their definitions into Chromium. i.e. you should just use the `android.text.ShowSecretsSetting` APIs in the implementation downstream.
Richard (Torne) ColesThanks for the pointers!
Few follow up questions:
The CL that exported the aconfig flag was merged yesterday. Do you know how long it might take for the flag to be available in the unreleased sdk in downstream?
To add the flag check like the way you mentioned, we need to write two CLs, one in the public chromium and one in the interal //clank. If we want to make the feature available by M145, is it a must to land both CLs by the M145 branch date or only merging the public CL by the branch date is sufficient? I am asking as I am not sure how long it might take for the flag to be available in downstream.
The SDK is autorolled weekly but the bots can be triggered manually any time - I just triggered the first step of the process and we should be able to roll to a new enough version in a couple of hours.
Both CLs would need to land before the M145 branch point, or be cherrypicked.
That's super helpful. Thanks! I will abandon this change and start working on the new ones.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Md Shahadat Hossain Shahin (Gerrit)
Md Shahadat Hossain Shahin added 1 comment![]( "Open in Gerrit")
return AconfigPackage.load("com.android.text.flags")Md Shahadat Hossain ShahinThis is the only usage of AconfigPackage in Chromium, and the [docs](https://developer.android.com/reference/android/os/flagging/AconfigPackage) mention:
Note: this is intended only to be used by generated code. To determine if a given flag is enabled in app code, the generated android flags should be used.
Are you sure this is the right way to do this? (I have no idea myself.) Potentially it's worth mailing Chromium / Chrome on Android development lists?
Richard (Torne) ColesThanks for taking a look. I am also unsure about this. We previously had a discussion with torne@ about checking aconfig flag value from Chrome. Looping him in for comments.
Md Shahadat Hossain ShahinWe have the unreleased Android SDK including the generated flags library referred to by the docs here available in the downstream //clank repository, and we have a mechanism set up to allow this to be used from upstream via an abstraction that is a no-op in public builds, and that's what you should be using here.
See https://source.chromium.org/chromium/chromium/src/+/main:base/android/java/src/org/chromium/base/AconfigFlaggedApiDelegate.java for the public interface defined upstream, and see the `AconfigFlaggedApiDelegateImpl` class in `//clank` for the actual implementations which check flags and call new APIs.
You shouldn't *just* be checking the flag here - you should also be using the actual framework APIs/constants to access the setting values you need, instead of copying their definitions into Chromium. i.e. you should just use the `android.text.ShowSecretsSetting` APIs in the implementation downstream.
Richard (Torne) ColesThanks for the pointers!
Few follow up questions:
The CL that exported the aconfig flag was merged yesterday. Do you know how long it might take for the flag to be available in the unreleased sdk in downstream?
To add the flag check like the way you mentioned, we need to write two CLs, one in the public chromium and one in the interal //clank. If we want to make the feature available by M145, is it a must to land both CLs by the M145 branch date or only merging the public CL by the branch date is sufficient? I am asking as I am not sure how long it might take for the flag to be available in downstream.
The SDK is autorolled weekly but the bots can be triggered manually any time - I just triggered the first step of the process and we should be able to roll to a new enough version in a couple of hours.
Both CLs would need to land before the M145 branch point, or be cherrypicked.
SDK version 14689923 which should contain all the changes related to this API is currently going through the autoroller: https://chrome-internal-review.googlesource.com/c/clank/internal/apps/+/8913355
Md Shahadat Hossain ShahinSo once that gets through the CQ you should be able to implement this using the real API downstream.
The CL for updating the sdk failed the CQ and was abandoned. I am not familiar with the failure or the next steps. Could you please take a look?
Related details
- Richard (Torne) Coles
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Richard (Torne) Coles (Gerrit)
Richard (Torne) Coles added 1 comment![]( "Open in Gerrit")
- Md Shahadat Hossain Shahin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Richard (Torne) Coles (Gerrit)
The SDK has now rolled; there was a bug in one of the AndroidX libraries we consume that conflicted with a change in the SDK, which is fixed now.