[WebView] Implement API for user-agent metadata overrides [chromium/src : main]
Victor Tan (Gerrit)
Attention is currently required from: Mike Taylor, Peter Pakkenberg.
1 comment:
Patchset:
Mike, could you review the behavior change for the cases we generate the user-agent client hints:
Peter, could you please take look at the api implementation?
thanks.
To view, visit change 4781571. To unsubscribe, or for help writing mail filters, visit settings.
Peter Pakkenberg (Gerrit)
Attention is currently required from: Mike Taylor, Victor Tan.
14 comments:
File android_webview/browser/aw_user_agent_metadata.h:
Patch Set #6, Line 13:
class AwUserAgentMetadata {Does this class serve any purpose other than being a namespace? I think you can drop it and just make the methods free functions in the `android_webview` namespace.
File android_webview/browser/aw_user_agent_metadata_unittest.cc:
Patch Set #6, Line 58:
TEST_F(AwUserAgentMetadataTest, TestJavaObjectCppObject) {Can you refactor these into individual tests for readability? The name of the test can then describe what specific case is being tested.
TEST_F(AwUserAgentMetadataTest, TestJavaObjectCppObjectBitness) {
const struct {
std::string input_bitness;
std::string expect_bitness;
} tests[] = {
{.input_bitness = "foo", .expect_bitness = ""},
{.input_bitness = "0", .expect_bitness = ""},
{.input_bitness = "8", .expect_bitness = "8"},
{.input_bitness = "16", .expect_bitness = "16"},
{.input_bitness = "32", .expect_bitness = "32"},
{.input_bitness = "64", .expect_bitness = "64"},
{.input_bitness = "128", .expect_bitness = "128"},
{.input_bitness = "256", .expect_bitness = "256"},
{.input_bitness = "1000", .expect_bitness = ""},
};
for (size_t i = 0; i < std::size(tests); ++i) {
blink::UserAgentMetadata ua_metadata;
ua_metadata.bitness = tests[i].input_bitness;
blink::UserAgentMetadata actual_metadata =
AwUserAgentMetadata::FromJavaAwUserAgentMetadata(
env(),
AwUserAgentMetadata::ToJavaAwUserAgentMetadata(env(), ua_metadata));
EXPECT_EQ(tests[i].expect_bitness, actual_metadata.bitness);
}
}Please write the tests individually, especially since you are mixing "good" and "bad" tests.
I would also expect us to parse any number correctly instead of limiting input to the enum values. The enum values should be a developer-facing convenience and not a strict requirement.
```
// In UserAgentMetadataTest
std::string RoundTripBitness(std::string_view input) {
return AwUserAgentMetadata::FromJavaAwUserAgentMetadata(
env(),
AwUserAgentMetadata::ToJavaAwUserAgentMetadata(env(), input));
}// Individual tests become
TEST_F(AwUserAgentMetadataTest, TestBitnessParsing_InvalidValue_String) {
EXPECT_EQ("", RoundTripBitness("foo"));
}
```
File android_webview/java/src/org/chromium/android_webview/AwSettings.java:
Patch Set #6, Line 749:
* See {@link android.webkit.WebSettings#setUserAgentMetadata}.Not sure about the link - did you mean to link to `androidx.webkit...`?
Patch Set #6, Line 752:
public void setUserAgentMetadata(Map<String, Object> uaMetadataMap) {could you make this one take an instance of `AwUserAgentMetadata` and move the parsing logic to something like `AwUserAgentMetadata.fromMap(Map<String, Object>)`? That way, all the to/from map logic is in the same place.
That would also make this method mirror the type signature of `getDefaultUserAgentMetadata`
Patch Set #6, Line 755:
final String oldUserAgentMetadata = mAwUserAgentMetadata.toString();Why are you comparing strings when you have implemented an `equals` method?
File android_webview/java/src/org/chromium/android_webview/client_hints/AwUserAgentMetadata.java:
Patch Set #6, Line 35:
public @interface Bitness {}Does anything break if you simply store an `int`? That would allow apps to specify any value. The enum can be a public API detail, and it would simplify the parsing significantly.
Patch Set #6, Line 175:
public void applyOverrides(@NonNull Map<String, Object> uaMetadataMap) {As with the `AwSettings` method, could this method be implemented as `applyOverrides(@NonNull AwUserAgentMetadata other)` and have the parsing from map in a separate method?
I can't tell how someone can clear an override for a specific key given this method. Is that an option?
Patch Set #6, Line 176:
for (Entry<String, Object> entry : uaMetadataMap.entrySet()) {I think it would be better to go through the known keys one by one instead of iterating over the set.
```
Object brandVersionValue = uaMetadataMap.get(METADATA_BRAND_VERSION_LIST);
if (brandValue != null) {
if (brandValue instanceof String[][]) {
// happy path, cast and use brandValue
} else {
throw new InvalidArgumentException(...);
}
}// Similar for the other values
```You can also write a few helper functions like this (using the suggestion from elsewhere to make the keys a @StringDef enum)
```
private String getValueAsString(Map<String, Object> map, @Keys String key) {
Object value = map.get(key);
if (value != null && !(value instanceof String)) {
throw new InvalidArgumentException("...");
}
return (String) value;
}
```Then you can write
```
String platform = getValueAsString(uaMetadataMap, Keys.PLATFORM);
if (platform != null) {
// ...
}
```
This means we don't iterate over a humongous map, but only access keys we expect. And the control flow becomes easier to read than a switch in a loop.Patch Set #6, Line 290:
public String toString() {If this is only for testing, you might want to put a specific serialiser in the test where it is used to not ship it in the binary.
Patch Set #6, Line 300:
equalsOverriding `equals` without `hashcode` can lead to some unexpected behaviour down the line.
You should either implement both or none.
File android_webview/java/src/org/chromium/android_webview/client_hints/AwUserAgentMetadataConstants.java:
Patch Set #6, Line 10:
AwUserAgentMetadataConstantsYou can probably declare these constants directly in UserAgentMetadata as an @IntDef and @StringDef respectively.
We cannot share them directly with AndroidX - you will have to redefine them in the AndroidX repository.
To still namespace them, you can do
```
@IntDef({Bitness.DEFAULT, Bitness.BITNESS_8,... })
interface Bitness {
public static final int DEFAULT = 0;
public static final int BITNESS_8 = 8;
...
}@StringDef({Keys.BRAND_VERSION_LIST, ...})
interface Keys {
public static final String BRAND_VERSION_LIST = "BRAND_VERSION_LIST";
...
}
```
File android_webview/java/src/org/chromium/android_webview/common/ProductionSupportedFlagList.java:
Flag.baseFeature(AwFeatures.WEBVIEW_ENABLE_OVERRIDE_USER_AGENT_CLIENT_HINTS,
"Enables override user-agent client hints in WebView."),We typically don't protect new API features behind feature flags. It creates a bad experience for app developers if they call a method and it doesn't do anything.
We have a method for adding new APIs as "hidden"/"disabled" that allows us to choose which binary version we enable them in.
To view, visit change 4781571. To unsubscribe, or for help writing mail filters, visit settings.
Victor Tan (Gerrit)
Attention is currently required from: Mike Taylor, Peter Pakkenberg.
14 comments:
File android_webview/browser/aw_user_agent_metadata.h:
Patch Set #6, Line 13:
class AwUserAgentMetadata {
Does this class serve any purpose other than being a namespace? I think you can drop it and just mak […]
only a couple of functions to translate the uametadata objects. I can drop the class.
File android_webview/browser/aw_user_agent_metadata_unittest.cc:
Patch Set #6, Line 58:
TEST_F(AwUserAgentMetadataTest, TestJavaObjectCppObject) {
Can you refactor these into individual tests for readability? The name of the test can then describe […]
will do
TEST_F(AwUserAgentMetadataTest, TestJavaObjectCppObjectBitness) {
const struct {
std::string input_bitness;
std::string expect_bitness;
} tests[] = {
{.input_bitness = "foo", .expect_bitness = ""},
{.input_bitness = "0", .expect_bitness = ""},
{.input_bitness = "8", .expect_bitness = "8"},
{.input_bitness = "16", .expect_bitness = "16"},
{.input_bitness = "32", .expect_bitness = "32"},
{.input_bitness = "64", .expect_bitness = "64"},
{.input_bitness = "128", .expect_bitness = "128"},
{.input_bitness = "256", .expect_bitness = "256"},
{.input_bitness = "1000", .expect_bitness = ""},
};
for (size_t i = 0; i < std::size(tests); ++i) {
blink::UserAgentMetadata ua_metadata;
ua_metadata.bitness = tests[i].input_bitness;
blink::UserAgentMetadata actual_metadata =
AwUserAgentMetadata::FromJavaAwUserAgentMetadata(
env(),
AwUserAgentMetadata::ToJavaAwUserAgentMetadata(env(), ua_metadata));
EXPECT_EQ(tests[i].expect_bitness, actual_metadata.bitness);
}
}
Please write the tests individually, especially since you are mixing "good" and "bad" tests. […]
will split them
File android_webview/java/src/org/chromium/android_webview/AwSettings.java:
Patch Set #6, Line 749:
* See {@link android.webkit.WebSettings#setUserAgentMetadata}.
Not sure about the link - did you mean to link to `androidx.webkit... […]
ok yea, should it be `androidx.webkit.WebSettingsCompat#setUserAgentMetadata`
Patch Set #6, Line 752:
public void setUserAgentMetadata(Map<String, Object> uaMetadataMap) {
could you make this one take an instance of `AwUserAgentMetadata` and move the parsing logic to some […]
sure,refactored
Patch Set #6, Line 755:
final String oldUserAgentMetadata = mAwUserAgentMetadata.toString();Why are you comparing strings when you have implemented an `equals` method?
refactor and no need to compare the hold object,
File android_webview/java/src/org/chromium/android_webview/client_hints/AwUserAgentMetadata.java:
Patch Set #6, Line 35:
public @interface Bitness {}
Does anything break if you simply store an `int`? That would allow apps to specify any value. […]
it doesn't break anything, in real, we accept any string value, no restriction on chromium side.
I can't tell how someone can clear an override for a specific key given this method. […]
hm, update the logic to accept null value for each setting, if it sets to null, we reset the setting to use default value, otherwise apply the overrides.
and added corresponding tests for clear an override.
Patch Set #6, Line 175:
public void applyOverrides(@NonNull Map<String, Object> uaMetadataMap) {
As with the `AwSettings` method, could this method be implemented as `applyOverrides(@NonNull AwUser […]
Done
Patch Set #6, Line 176:
for (Entry<String, Object> entry : uaMetadataMap.entrySet()) {
I think it would be better to go through the known keys one by one instead of iterating over the set […]
sure, will refactor those.
Patch Set #6, Line 290:
public String toString() {
If this is only for testing, you might want to put a specific serialiser in the test where it is use […]
removed.
Patch Set #6, Line 300:
equals
Overriding `equals` without `hashcode` can lead to some unexpected behaviour down the line. […]
no need to compare objects.
File android_webview/java/src/org/chromium/android_webview/client_hints/AwUserAgentMetadataConstants.java:
Patch Set #6, Line 10:
AwUserAgentMetadataConstants
You can probably declare these constants directly in UserAgentMetadata as an @IntDef and @StringDef […]
good to know, will move them to UserAgentMetadata
File android_webview/java/src/org/chromium/android_webview/common/ProductionSupportedFlagList.java:
Flag.baseFeature(AwFeatures.WEBVIEW_ENABLE_OVERRIDE_USER_AGENT_CLIENT_HINTS,
"Enables override user-agent client hints in WebView."),
We typically don't protect new API features behind feature flags. […]
sure, will consider remove them and control over on API level.
To view, visit change 4781571. To unsubscribe, or for help writing mail filters, visit settings.
Peter Pakkenberg (Gerrit)
Attention is currently required from: Mike Taylor, Victor Tan.
17 comments:
File android_webview/browser/aw_user_agent_metadata.cc:
Patch Set #8, Line 61:
ConvertJavaStringToUTF8`ConvertJavaStringToUTF8` fails if you pass a `null` string. The Java getters all simply return the stored field values, which can be `null`, so you should check for null before trying to convert to a `std::string`.
Patch Set #8, Line 63:
if (!full_version.empty()) {Since this is a locally created object, I would assume that the default value of the string members is already empty string, and assigning a new empty string changes nothing. Could we do away with these checks in that case?
File android_webview/java/src/org/chromium/android_webview/AwSettings.java:
Patch Set #8, Line 754:
setUserAgentMetadataAs with the getter, consider renaming to `setUserAgentMetadataFromMap` to distinguish this from a normal setter.
Patch Set #8, Line 780:
getUserAgentMetadataIt is a bit confusing from the name that this method does not return an instance of `AwUserAgentMetadata`.
Is there any harm in letting it return the object and letting callers decide if they need the map?
If there is, consider renaming it to `getUserAgentMetadataMap`.
File android_webview/java/src/org/chromium/android_webview/client_hints/AwUserAgentMetadata.java:
Patch Set #8, Line 15:
import java.security.InvalidParameterException;Consider using `java.lang.IllegalArgumentException` instead, which is the canonical exception to use when receiving invalid input.
[InvalidParameterException] has a very specific meaning.
[InvalidParameterException]: https://docs.oracle.com/javase%2F7%2Fdocs%2Fapi%2F%2F/java/security/InvalidParameterException.htmlPatch Set #8, Line 31:
private String[][] mBrandVersionList;Is there any chance this could be `Map<String, java.android.Pair<String, String>>` instead? Or does that make everything much worse in terms of readability?
Patch Set #8, Line 108:
public AwUserAgentMetadata(AwUserAgentMetadata other) {copy constructors is less of an idiom in Java than C++ - it may be more explicit to make this something like
```
public AwUserAgentMetadata deepCopy() {
// return a deep copy of the object
}
```Patch Set #8, Line 109:
other.mBrandVersionListThis will assign a reference to the same in-memory array, which could be modified elsewhere. I recommend that you defensively create copies of arrays received from elsewhere (especially from the embedding app) to avoid this creating hard to find bugs.
I've put the comment here, but it applies elsewhere in this class too, especially in `fromMap`.
Patch Set #8, Line 204:
return Optional.ofNullable((String) value).orElse(defaultValue);I'm not convinced this is better than simply
```
if (value != null) {
return (String) value;
} else {
return defaultValue;
}
```At a minimum, you are asking the JIT compiler to handle two method calls and the creation of an object, which it _may_ be able to optimise away, but is definitely not guaranteed to do.
Same for the other methods below.
Patch Set #8, Line 255:
for (int i = 0; i < brandVersionList.length; i++) {You can use [enhanced for loops] with Java arrays instead of indexed looping. You don't seem to be using the loop index anywhere:
```
for (String[] brandVersionInfo : brandVersionList) {
// ... use brandVersionInfo
}
```[enhanced for loops]: https://docs.oracle.com/javase/1.5.0/docs/guide/language/foreach.html
Patch Set #8, Line 270:
brandVersionList = defaultData.mBrandVersionList;You could instantiate `brandVersionList` with this value on line 248 and override it if there is a value in the map. That way, you don't risk it being `null` if the code changes in the future.
return new AwUserAgentMetadata(brandVersionList,
getValueAsString(
uaMetadataMap, MetadataKeys.FULL_VERSION, defaultData.mFullVersion),
getValueAsString(uaMetadataMap, MetadataKeys.PLATFORM, defaultData.mPlatform),
getValueAsString(
uaMetadataMap, MetadataKeys.PLATFORM_VERSION, defaultData.mPlatformVersion),
getValueAsString(
uaMetadataMap, MetadataKeys.ARCHITECTURE, defaultData.mArchitecture),
getValueAsString(uaMetadataMap, MetadataKeys.MODEL, defaultData.mModel),
getValueAsBoolean(uaMetadataMap, MetadataKeys.MOBILE, defaultData.mMobile),
getValueAsInt(uaMetadataMap, MetadataKeys.BITNESS, defaultData.mBitness),
getValueAsBoolean(uaMetadataMap, MetadataKeys.WOW64, defaultData.mWow64),
getValueAsString(uaMetadataMap, MetadataKeys.FORM_FACTOR, defaultData.mFormFactor))I am not thrilled by the readability of this block - it seems very easy to accidentally reorder some of the constructor parameters, especially because they are nearly all `String`.
Since all the constructors that take parameters are private anyway, do you think it becomes more readable if you change the constructor to not take any parameters and then do field assignments here:
```
AwUserAgentMetadata result = new AwUserAgentMetadata();
result.mFullVersion = getValueAsString(uaMetadataMap, MetadataKeys.FULL_VERSION, defaultData.mFullVersion);
// ... same for the other fields
```Patch Set #8, Line 296:
equalsthis should be `Arrays.deepEquals`.
Patch Set #8, Line 297:
mBrandVersionList = overrideData.mBrandVersionList;Consider defensively copying (in which case you should check for equality before proceeding, unlike the rest of the fields).
if (!Objects.equals(mFullVersion, overrideData.mFullVersion)) {
mFullVersion = overrideData.mFullVersion;
needUpdate = true;
}Consider if
```
needsUpdate |= !Objects.equals(mFullVersion, overrideData.mFullVersion));
mFullVersion = overrideData.mFullVersion;
```reads better to you. Strings are immutable and everything is pointers, so assignments is very cheap, even if we are assigning the same value.
Same for all the other fields.
File android_webview/javatests/src/org/chromium/android_webview/test/ClientHintsTest.java:
Patch Set #8, Line 398:
ExceptionYou should only catch the expected exception type here, to assert that we are throwing the type we expect
`catch (IllegalArgumentException e)`
Patch Set #8, Line 412:
// Override with bitness 0, we expect it return an empty string.Do we really expect an empty string? I would expect the bitness of the platform to be the default value sent by the user agent when the client specifies `DEFAULT`.
But, I can very likely have misunderstood something.
To view, visit change 4781571. To unsubscribe, or for help writing mail filters, visit settings.
Victor Tan (Gerrit)
Attention is currently required from: Mike Taylor, Peter Pakkenberg.
17 comments:
File android_webview/browser/aw_user_agent_metadata.cc:
Patch Set #8, Line 61:
ConvertJavaStringToUTF8
`ConvertJavaStringToUTF8` fails if you pass a `null` string. […]
the result will be empty string if null pass, https://source.chromium.org/chromium/chromium/src/+/main:base/android/jni_string.cc;drc=93f41097bb9b07efd4b8e7e4536b8bf920baa915;bpv=1;bpt=1;l=28, I can wrapper it to another function to process a null to empty string.
Patch Set #8, Line 63:
if (!full_version.empty()) {
Since this is a locally created object, I would assume that the default value of the string members […]
sure, will remove the check.
File android_webview/java/src/org/chromium/android_webview/AwSettings.java:
Patch Set #8, Line 754:
setUserAgentMetadata
As with the getter, consider renaming to `setUserAgentMetadataFromMap` to distinguish this from a no […]
will rename it.
Patch Set #8, Line 780:
getUserAgentMetadata
It is a bit confusing from the name that this method does not return an instance of `AwUserAgentMeta […]
updated the name, also, IIUC, this is the public API to get the user-agent metadata, if we can return the object for get API, why we should better pass Map (instead of object) to the setUserAgentMetadata API?
File android_webview/java/src/org/chromium/android_webview/client_hints/AwUserAgentMetadata.java:
Patch Set #8, Line 15:
import java.security.InvalidParameterException;
Consider using `java.lang. […]
will do
Patch Set #8, Line 31:
private String[][] mBrandVersionList;
Is there any chance this could be `Map<String, java.android. […]
using a map can't guarantee the order of users's overrides brand list,
also, it might not easily to extend more field on the brand.
Patch Set #8, Line 108:
public AwUserAgentMetadata(AwUserAgentMetadata other) {
copy constructors is less of an idiom in Java than C++ - it may be more explicit to make this someth […]
here is only the shallow copy of the instance, we will only do deep copy once the arrays received from outside are different from the existing settings.
Patch Set #8, Line 109:
other.mBrandVersionList
This will assign a reference to the same in-memory array, which could be modified elsewhere. […]
will do the deepcopy arrays defensively.
Patch Set #8, Line 204:
return Optional.ofNullable((String) value).orElse(defaultValue);
I'm not convinced this is better than simply […]
Done
Patch Set #8, Line 255:
for (int i = 0; i < brandVersionList.length; i++) {
You can use [enhanced for loops] with Java arrays instead of indexed looping. […]
Acknowledged
Patch Set #8, Line 270:
brandVersionList = defaultData.mBrandVersionList;
You could instantiate `brandVersionList` with this value on line 248 and override it if there is a v […]
Acknowledged
return new AwUserAgentMetadata(brandVersionList,
getValueAsString(
uaMetadataMap, MetadataKeys.FULL_VERSION, defaultData.mFullVersion),
getValueAsString(uaMetadataMap, MetadataKeys.PLATFORM, defaultData.mPlatform),
getValueAsString(
uaMetadataMap, MetadataKeys.PLATFORM_VERSION, defaultData.mPlatformVersion),
getValueAsString(
uaMetadataMap, MetadataKeys.ARCHITECTURE, defaultData.mArchitecture),
getValueAsString(uaMetadataMap, MetadataKeys.MODEL, defaultData.mModel),
getValueAsBoolean(uaMetadataMap, MetadataKeys.MOBILE, defaultData.mMobile),
getValueAsInt(uaMetadataMap, MetadataKeys.BITNESS, defaultData.mBitness),
getValueAsBoolean(uaMetadataMap, MetadataKeys.WOW64, defaultData.mWow64),
getValueAsString(uaMetadataMap, MetadataKeys.FORM_FACTOR, defaultData.mFormFactor))
I am not thrilled by the readability of this block - it seems very easy to accidentally reorder some […]
Done
Patch Set #8, Line 296:
equalsthis should be `Arrays.deepEquals`.
Done
Patch Set #8, Line 297:
mBrandVersionList = overrideData.mBrandVersionList;
Consider defensively copying (in which case you should check for equality before proceeding, unlike […]
Done
if (!Objects.equals(mFullVersion, overrideData.mFullVersion)) {
mFullVersion = overrideData.mFullVersion;
needUpdate = true;
}
Consider if […]
Done
File android_webview/javatests/src/org/chromium/android_webview/test/ClientHintsTest.java:
Patch Set #8, Line 398:
Exception
You should only catch the expected exception type here, to assert that we are throwing the type we e […]
Done
Patch Set #8, Line 412:
// Override with bitness 0, we expect it return an empty string.
Do we really expect an empty string? I would expect the bitness of the platform to be the default va […]
empty string is the default value on android and webview for bitness.
To view, visit change 4781571. To unsubscribe, or for help writing mail filters, visit settings.
Peter Pakkenberg (Gerrit)
Attention is currently required from: Mike Taylor, Richard Coles, Victor Tan.
6 comments:
File android_webview/browser/aw_user_agent_metadata.cc:
Patch Set #10, Line 71:
ua_metadata.full_version = full_version;This may just be a matter of style opinion, so feel free to disregard, but is there any reason why you introduce a local variable instead of assigning directly to the struct member? If you are doing it for readability, then that is fine from my perspective.
The compiler will _probably_ inline for you, but I am by no means enough of an expert in C++ to debate if there are any performance differences?
(same for rest of the function)
File android_webview/java/src/org/chromium/android_webview/AwSettings.java:
Patch Set #8, Line 780:
getUserAgentMetadataupdated the name, also, IIUC, this is the public API to get the user-agent metadata, if we can retur […]
This is the Chrome-internal API. The developer-facing API lives in [WebSettingsCompat] in the AndroidX library, where we can wrap this in a more developer-friendly class.
Because of the restriction of the AndroidX library, where we can only pass types that are part of the Android SDK classloader, the map of objects makes the most sense as a serialisation format across this barrier.
[WebSettingsCompat]: https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:webkit/webkit/src/main/java/androidx/webkit/WebSettingsCompat.java
File android_webview/java/src/org/chromium/android_webview/AwSettings.java:
Patch Set #10, Line 759:
if (mAwUserAgentMetadata.applyOverrides(overrideUaMetadata)) {At this point, is there any difference between the current code and something like
```
if (!mAwUserAgentMetadata.equals(overrideUaMetadata)) {
mAwUserAgentMetadata = overrideUaMetadata;
/// rest as before
```Is there any value in keeping the `mAwUserAgentMetadata` as the same object?
That would mean that `applyUpdates` could be just an `equals` (or some other name to avoid having to also implement `hashcode`, as per my previous comments) but would only do the comparison, reducing the complexity of that method.
The only semantic difference I can see right now is that `applyUpdates` creates a copy of the brand version list, but that copy could be moved to the `fromMap` method.
File android_webview/java/src/org/chromium/android_webview/client_hints/AwUserAgentMetadata.java:
Patch Set #10, Line 91:
private AwUserAgentMetadata() {It may be a good idea to add a comment to this constructor to explain why we are using a no-args constructor for this class (namely, that the constructor ends up with a confusing amount of String arguments).
Just to make it clear to the next person who sees the code that they shouldn't refactor it back to a constructor with args.
Patch Set #10, Line 92:
super();You shouldn't need to explicitly call the no-args super constructor.
Patch Set #10, Line 189:
return new AwUserAgentMetadata(brandVersionList, brandFullVersionList, fullVersion,Could you use the no-args constructor here as well and move the logic from the constructor you are calling into this method to again make it clearer which arguments are being passed where?
We do still have the problem on the native side where this method is called, but I think it's worth trying to minimise it as much as possible.
@to...@chromium.org would you mind offering your opinion on readability here as well? My concern is that having almost all the method parameters to the constructor be strings makes it very easy to accidentally mess up the order.
To view, visit change 4781571. To unsubscribe, or for help writing mail filters, visit settings.
Victor Tan (Gerrit)
Attention is currently required from: Mike Taylor, Peter Pakkenberg, Richard Coles.
5 comments:
File android_webview/browser/aw_user_agent_metadata.cc:
Patch Set #10, Line 71:
ua_metadata.full_version = full_version;
This may just be a matter of style opinion, so feel free to disregard, but is there any reason why y […]
hm, will update to assign it directly, i don't think there are any performance difference, this kind of style inherit from the previous revision.
File android_webview/java/src/org/chromium/android_webview/AwSettings.java:
Patch Set #10, Line 759:
if (mAwUserAgentMetadata.applyOverrides(overrideUaMetadata)) {
At this point, is there any difference between the current code and something like […]
another difference is that we will always make a deep copy for the 2d array no matter whether the brand list are different with the existing settings or not.
I can refactor them to an equal+assign if we don't care about such the difference.
File android_webview/java/src/org/chromium/android_webview/client_hints/AwUserAgentMetadata.java:
Patch Set #10, Line 91:
private AwUserAgentMetadata() {
It may be a good idea to add a comment to this constructor to explain why we are using a no-args con […]
combining with other comments, will remove the default no-args construct since there is no string args constructors, take it as the default no-args.
Patch Set #10, Line 92:
super();You shouldn't need to explicitly call the no-args super constructor.
Acknowledged
Patch Set #10, Line 189:
return new AwUserAgentMetadata(brandVersionList, brandFullVersionList, fullVersion,
Could you use the no-args constructor here as well and move the logic from the constructor you are c […]
refactor all as no-args constructor.
To view, visit change 4781571. To unsubscribe, or for help writing mail filters, visit settings.
Peter Pakkenberg (Gerrit)
Attention is currently required from: Mike Taylor, Richard Coles, Victor Tan.
Patch set 11:Code-Review +1
2 comments:
Patchset:
LGTM with the last comment. I have added torne@ as reviewer for //android_webview
File android_webview/java/src/org/chromium/android_webview/client_hints/AwUserAgentMetadata.java:
Patch Set #10, Line 92:
super();
You shouldn't need to explicitly call the no-args super constructor.
I would still keep the constructor as `private` to avoid anyone else accidentally creating instances of this class.
To view, visit change 4781571. To unsubscribe, or for help writing mail filters, visit settings.
Victor Tan (Gerrit)
Attention is currently required from: Mike Taylor, Richard Coles.
1 comment:
File android_webview/java/src/org/chromium/android_webview/client_hints/AwUserAgentMetadata.java:
Patch Set #10, Line 92:
super();
I would still keep the constructor as `private` to avoid anyone else accidentally creating instances […]
done
To view, visit change 4781571. To unsubscribe, or for help writing mail filters, visit settings.
Mike Taylor (Gerrit)
Attention is currently required from: Richard Coles, Victor Tan.
Patch set 12:Code-Review +1
1 comment:
Patchset:
components/embedder_support/* LGTM
To view, visit change 4781571. To unsubscribe, or for help writing mail filters, visit settings.
Richard Coles (Gerrit)
Attention is currently required from: Victor Tan.
Patchset:
LGTM for //android_webview OWNERS.
I discussed this with Victor yesterday, but for the record: this change may have an undesired effect on existing apps that are setting their useragent string to something custom that does not include the default UA. With the existing code that's currently being experimented, these apps will have empty values for the JS UACH APIs, but this CL changes that to instead populate the low-entropy values as normal.
This avoids the issue of pages being confused by the empty values in the API (which we know is an issue for some pages, as it's why we disabled the UACH API in webview in the first place), but instead creates a different issue if the reason the app is changing its useragent is to *conceal* that it's WebView, or impersonate another browser, since the UACH API values will not match the UA string and potentially reveal what the app is doing. Once we ship the API to override the UACH metadata, these apps can also override that to avoid the problem, but existing apps may not be updated.
I'm okay to go ahead with this change, because fixing the empty UACH data seems more important and there will be options for apps to override it once the API is shipped, but we should be careful how it rolls out.
To view, visit change 4781571. To unsubscribe, or for help writing mail filters, visit settings.
Andrey Zaytsev (Gerrit)
Victor Tan (Gerrit)
Attention is currently required from: Victor Tan.
Patch set 13:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
12 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
[WebView] Implement API for user-agent metadata overrides
To support better explore user-agent client hints, this CL implements
API to overrides the user-agent metadata which used to generate the
user-agent client hints for requests. We also provide a get API to check
current user-agent metadata. This CL only include changes on Chromium
and WebView, follow-up CLs will include boundary interface changes.
This CL also changes the behavior of generating user-agent client hints
when user totally change the user-agent without providing any overrides
for user-agent metadata. In other words, if the custom user-agent
doesn't contains system default user-agent, we only populate the
low-entropy user-agent client hints (sec-ch-ua, sec-ch-ua-platform,
sec-ch-ua-mobile) for the requests, high-entropy client hints will be
empty.
The major logic to generate user-agent metadata:
1. user provide user-agent metadata overrides, apply what users overrides.
2. user doesn't user-agent metadata overrides, but user-agent contains
system default user-agent, using system default user-agent metadata
to generate user-agent client hints.
3. otherwise, only low-entropy user-agent client hints populate.
We also add a metric to track different cases of generating user-agent
metadata to help us better understand how the overrides works.
OBSOLETE_HISTOGRAM[Android.WebView.UserAgentClientHintsMetadata.Available name]=prefer to use the new metrics Android.WebView.UserAgentClientHintsMetadata.AvailableType
Change-Id: I94cba376fbcfecff2d8db921e59bfe1e819f24f3
Bug: b/294183509
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4781571
Commit-Queue: Victor Tan <vict...@chromium.org>
Reviewed-by: Peter Pakkenberg <pb...@chromium.org>
Reviewed-by: Mike Taylor <mike...@chromium.org>
Reviewed-by: Richard Coles <to...@chromium.org>
Reviewed-by: Andrey Zaytsev <andza...@google.com>
Cr-Commit-Position: refs/heads/main@{#1188635}
---
M android_webview/BUILD.gn
M android_webview/browser/BUILD.gn
M android_webview/browser/aw_client_hints_controller_delegate.cc
M android_webview/browser/aw_client_hints_controller_delegate.h
M android_webview/browser/aw_client_hints_controller_delegate_unittest.cc
M android_webview/browser/aw_settings.cc
A android_webview/browser/aw_user_agent_metadata.cc
A android_webview/browser/aw_user_agent_metadata.h
A android_webview/browser/aw_user_agent_metadata_unittest.cc
M android_webview/java/src/org/chromium/android_webview/AwSettings.java
A android_webview/java/src/org/chromium/android_webview/client_hints/AwUserAgentMetadata.java
M android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
M android_webview/javatests/src/org/chromium/android_webview/test/ClientHintsTest.java
M android_webview/test/BUILD.gn
M components/embedder_support/user_agent_utils.cc
M components/embedder_support/user_agent_utils.h
M components/embedder_support/user_agent_utils_unittest.cc
M tools/metrics/histograms/enums.xml
M tools/metrics/histograms/metadata/android/histograms.xml
19 files changed, 1,560 insertions(+), 74 deletions(-)