Android autoplay #10120

wchen342 · 2021-09-16T16:02:47Z

Resolves brave/brave-browser/issues/14142.

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

The setting spans multiple places. Test as the following:

Open "Settings - Site settings - Autoplay" and toggle off to "Blocked"
Go to any site with embedded video, make sure the video doesn't play.
Wait until the webpage is fully loaded, then click on the lock icon on the left side of omnibox.
From the popup menu, choose "Permissions - Autoplay", make sure the slide button is at "Blocked"
Toggle the slide button to be "Allowed"
Refresh the page, make sure the video now starts to play on itself
Go back to "Settings - Site settings - Autoplay", make sure an exception is added for the site you just used

samartnik · 2021-09-16T16:26:36Z

build/android/bytecode/java/org/brave/bytecode/BraveClassVisitor.java

@@ -159,6 +168,10 @@ private boolean shouldMakePublicMethod(String className, String methodName) {
                mMakePublicMethods.entrySet()) {
            String entryClassName = entry.getKey();
            ArrayList<String> methodNames = entry.getValue();
+            // Why className.contains(entryClassName)? This seems erroneous


Yes, it seems like we do need to change it to equals. Did you get that warning for any change?

samartnik · 2021-09-16T16:31:39Z

build/android/bytecode/java/org/brave/bytecode/BraveClassVisitor.java

@@ -252,6 +290,20 @@ protected void deleteInnerClass(String outerName, String innerName) {
        innerNames.add(innerName);
    }

+    private boolean shouldMakeNonFinalClass(String className) {
+        int idx = mMakeNonFinalClasses.indexOf(className);


Isn't there contains method for that?

samartnik · 2021-09-16T16:38:19Z

...java/src/org/chromium/components/browser_ui/site_settings/BraveContentSettingsResources.java

+        }
+
+        return (ResourceItem) BraveReflectionUtil.InvokeMethod(
+                ContentSettingsResources.class, null, "getResourceItem", int.class, contentType);


Did you check it in release? We should to allow list it in brave/android/java/proguard.flags. And we should add it to testMethodsForInvocationExist test to check that exact params and return type are not changed.
It applies to all calls via BraveReflectionUtil.InvokeMethod, I just don't want to duplicate this message for all of them.

I added those to testMethodsExist but yeah that one doesn't test for parameter types. I will change to testMethodsForInvocationExist.

samartnik · 2021-09-16T16:51:32Z

build/android/bytecode/java/org/brave/bytecode/BraveContentSettingsResourcesClassAdapter.java

+        makePublicMethod(sContentSettingsResourcesClassName, "getResourceItem");
+        changeMethodOwner(
+                sContentSettingsResourcesClassName, "getResourceItem", sBraveContentSettingsResourcesClassName);
+        makePublicInnerClass(sContentSettingsResourcesClassName, "ResourceItem");


We need to add a test that checks that this inner class is still there.

samartnik · 2021-09-16T16:53:27Z

build/android/bytecode/java/org/brave/bytecode/BraveContentSettingsResourcesClassAdapter.java

+                sContentSettingsResourcesClassName, "getResourceItem", sBraveContentSettingsResourcesClassName);
+        makePublicInnerClass(sContentSettingsResourcesClassName, "ResourceItem");
+        redirectConstructor(sBraveContentSettingsResourcesResourceItemClassName, sContentSettingsResourcesResourceItemClassName);
+        redirectTypeInMethod("getResourceItem", sBraveContentSettingsResourcesResourceItemClassName, sContentSettingsResourcesResourceItemClassName);


We need to add a test that checks that new type is actually applied.

samartnik · 2021-09-16T16:55:02Z

build/android/bytecode/java/org/brave/bytecode/BraveSingleCategorySettingsClassAdapter.java

@@ -15,5 +15,8 @@ public BraveSingleCategorySettingsClassAdapter(ClassVisitor visitor) {
        super(visitor);

        changeSuperName(sSingleCategorySettingsClassName, sBraveSingleCategorySettingsClassName);
+        changeMethodOwner(sSingleCategorySettingsClassName, "onOptionsItemSelected", sBraveSingleCategorySettingsClassName);


I don't see unit test for this change.

samartnik · 2021-09-16T16:57:11Z

build/android/bytecode/java/org/brave/bytecode/BraveSingleWebsiteSettingsClassAdapter.java

+    public BraveSingleWebsiteSettingsClassAdapter(ClassVisitor visitor) {
+        super(visitor);
+
+        changeSuperName(sSingleWebsiteSettingsClassName, sBraveSingleWebsiteSettingsClassName);


We need to add test to testSuperNames for this.
Generally, just to not duplicate similar message, could you please double check that every change in bytecode has its test in brave/android/javatests/org/chromium/chrome/browser/BytecodeTest.java.

samartnik · 2021-09-21T13:46:14Z

android/java/proguard.flags

@@ -8,3 +8,26 @@
 -keep class org.chromium.chrome.browser.ChromeTabbedActivity {
    *** hideOverview(...);
 }
+
+-keep class org.chromium.components.browser_ui.site_settings.ContentSettingsResources {
+    private static *** getResourceItem(...);


We should generalize it, as we don't care whether it's private or not. And if they change it, we will simply get crash on trying to invoke it.
Same applies to static, we should add unit test instead to check that it is still static, as they may change it and we will get crash on trying to invoke it.

And we should also add tests that non-static functions remain non-static. As, if they change it in upstream, current tests will pass, but we will get crashes in runtime on trying to invoke them. This applies to hideOverview as well, we overlooked this moment before.

samartnik · 2021-09-21T13:48:06Z

android/javatests/org/chromium/chrome/browser/BytecodeTest.java

        // NOTE: Add new checks above. For each new check in this method add proguard exception in
        // `brave/android/java/proguard.flags` file under `Add methods for invocation below`
        // section. Both test and regular apks should have the same exceptions.
+        Assert.assertTrue(


Comment says to put it all above.

samartnik · 2021-09-21T13:49:39Z

build/android/bytecode/java/org/brave/bytecode/BraveClassVisitor.java

@@ -159,6 +166,10 @@ private boolean shouldMakePublicMethod(String className, String methodName) {
                mMakePublicMethods.entrySet()) {
            String entryClassName = entry.getKey();
            ArrayList<String> methodNames = entry.getValue();
+            // Why className.contains(entryClassName)? This seems erroneous
+            if (methodNames.contains(methodName) && className.equals(entryClassName) != className.contains(entryClassName)) {
+                   System.out.println("Warning: class name " + className + " may not be written correctly!");


Do you get this warning? If not, we just need to change className.contains to className.equals below.

bridiver · 2021-09-21T17:45:25Z

patches/components-browser_ui-site_settings-android-website_preference_bridge.cc.patch

+       case ContentSettingsType::POPUPS:
+       case ContentSettingsType::SENSORS:
+       case ContentSettingsType::SOUND:
+      case ContentSettingsType::AUTOPLAY:


this can be done with a chromium src override
define AUTOMATIC_DOWNLOADS ContentSettingsType::AUTOPLAY: value = CONTENT_SETTINGS_ALLOW; break; AUTOMATIC_DOWNLOADS

Doesn't that still require a patch? Or is it for maintainability in the future?

it does not require a patch and it is extensible unlike this patch. Even if we patched, we wouldn't patch like this because this is not extensible and does not follow the patching guidelines

bridiver · 2021-09-21T18:16:56Z

...id-java-src-org-chromium-components-browser_ui-site_settings-SiteSettingsCategory.java.patch

+        int NUM_ENTRIES = 24;
+     }
+
+     private final BrowserContextHandle mBrowserContextHandle;


I believe we have a way to replace static method calls with calls to a different method so can do BraveSiteSettingsCategory.contentSettingsType which calls SiteSettingsCategory.contentSettingsType and then checks for autoplay if the return is -1

That's what I did with the rest of the java codes but it is impossible to do here because (1) the definition is annotated and to redirect the definition we need to do some very hacky bytecode manipulation (which I am not sure is possible at all), and if the descendant class has values not in the annotation compiler will probably complain too. (2) the value is extensive used in site_settings and some of them are not static, which means I need to do the hack of changing all type definitions in a function body with bytecode on all of them, which IMO is worse than just using a patch.

(I have asked @samartnik and he agree this part is not possible with bytecode).

bridiver · 2021-09-21T18:17:06Z

...id-java-src-org-chromium-components-browser_ui-site_settings-SiteSettingsCategory.java.patch

+                 return "use_storage";
+             case Type.VIRTUAL_REALITY:
+                 return "virtual_reality";
+            case Type.AUTOPLAY:


same as above

SergeyZhukovsky · 2021-09-21T20:28:49Z

browser/ui/android/strings/android_brave_strings.grd

+      </message>
+      <message name="IDS_WEBSITE_SETTINGS_ADD_SITE_DESCRIPTION_AUTOPLAY" desc="The description for the allow autoplay of muted videos for website dialog.">
+        Allow autoplay of muted videos for a specific site.
+      </message>


Did we get the above strings from the old code?

Yeah that's from old version.

bridiver · 2021-09-21T23:11:52Z

...id-java-src-org-chromium-components-browser_ui-site_settings-SiteSettingsCategory.java.patch

+                 return ContentSettingsType.VR;
+             // case Type.ALL_SITES
+             // case Type.USE_STORAGE
+            case Type.AUTOPLAY:


these last two patches can be one line each

samartnik

lgtm

wchen342 added CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-ios Do not run CI builds for iOS labels Sep 16, 2021

wchen342 requested review from samartnik and a team as code owners September 16, 2021 16:02

wchen342 force-pushed the android_autoplay branch 3 times, most recently from f6b5914 to 4dc765b Compare September 16, 2021 16:13

samartnik requested changes Sep 16, 2021

View reviewed changes

wchen342 requested a review from samartnik September 17, 2021 14:46

wchen342 force-pushed the android_autoplay branch 2 times, most recently from ea3b9bb to 4e6ebda Compare September 21, 2021 13:17

samartnik requested changes Sep 21, 2021

View reviewed changes

bridiver reviewed Sep 21, 2021

View reviewed changes

SergeyZhukovsky reviewed Sep 21, 2021

View reviewed changes

bridiver reviewed Sep 21, 2021

View reviewed changes

wchen342 requested review from a team as code owners September 24, 2021 13:31

wchen342 requested a review from samartnik September 24, 2021 13:35

wchen342 force-pushed the android_autoplay branch from c23bdb2 to acd416e Compare September 24, 2021 14:19

samartnik approved these changes Sep 24, 2021

View reviewed changes

bridiver approved these changes Sep 27, 2021

View reviewed changes

wchen342 added 5 commits September 27, 2021 21:38

Add per site UI

5341261

Reform

b007339

Reflection update; change BraveSingleCategorySettings super

0ac0583

Add BraveWebsite, BraveSingleWebsiteSettings

3a50e72

Fixes

8e8c47a

wchen342 added 2 commits September 27, 2021 21:39

Add bytecode tests

4117806

patch and bytecode test fixes

d21977e

wchen342 force-pushed the android_autoplay branch from acd416e to 0d13a19 Compare September 27, 2021 19:18

Format fix

abcd7cf

wchen342 force-pushed the android_autoplay branch from 0d13a19 to abcd7cf Compare September 27, 2021 20:00

wchen342 merged commit ed561f4 into master Sep 27, 2021

wchen342 deleted the android_autoplay branch September 27, 2021 21:56

bsclifton added this to the 1.32.x - Nightly milestone Sep 28, 2021

SergeyZhukovsky assigned wchen342 Oct 19, 2021

stephendonner mentioned this pull request Nov 8, 2021

Android Autoplay should parity Desktop brave/brave-browser#14142

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android autoplay #10120

Android autoplay #10120

wchen342 commented Sep 16, 2021 •

edited

Loading

samartnik Sep 16, 2021

samartnik Sep 16, 2021

samartnik Sep 16, 2021

wchen342 Sep 16, 2021 •

edited

Loading

samartnik Sep 16, 2021

samartnik Sep 16, 2021

samartnik Sep 16, 2021

samartnik Sep 16, 2021

samartnik Sep 21, 2021

samartnik Sep 21, 2021

samartnik Sep 21, 2021

samartnik Sep 21, 2021

bridiver Sep 21, 2021

wchen342 Sep 21, 2021

bridiver Sep 21, 2021

bridiver Sep 21, 2021

wchen342 Sep 21, 2021 •

edited

Loading

bridiver Sep 21, 2021

SergeyZhukovsky Sep 21, 2021

wchen342 Sep 22, 2021

bridiver Sep 21, 2021

samartnik left a comment

Android autoplay #10120

Android autoplay #10120

Conversation

wchen342 commented Sep 16, 2021 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wchen342 Sep 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wchen342 Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samartnik left a comment

Choose a reason for hiding this comment

wchen342 commented Sep 16, 2021 •

edited

Loading

wchen342 Sep 16, 2021 •

edited

Loading

wchen342 Sep 21, 2021 •

edited

Loading