From 3e238531f7a0003dc7edb449b944ff4292423716 Mon Sep 17 00:00:00 2001 From: AlexeyBarabash Date: Mon, 26 May 2025 19:22:25 +0300 Subject: [PATCH] [cr138][Android] Fixed app menu and pull to refresh With Brave asm patching classes inheritence is ``` ChromeTabbedActivity extends BraveActivity BraveActivity extends ChromeActivity ``` With a new argument at `ChromeTabbedActivity.onMenuOrKeyboardAction` line `return super.onMenuOrKeyboardAction(id, fromMenu, triggeringMotion);` invoked not BraveActivity's method but ChromeActivity's one. This broke app menu and pull to refresh gesture. Compiler didn't give any warnings, so I added a testcase. Chromium changes: https://source.chromium.org/chromium/chromium/src/+/34d6b8ab128e787fdbf4c8bcf93e23ece93c0aa0 Pass triggeringMotionEvent from AppMenu to onMenuOrKeyboardAction() This CL is a no-op. It only changes method signatures. Context: * Tab closure will have different behavior depending on whether a click came from a peripheral. For example, when using a peripheral to close all tabs via the app menu, we don't want to show the "undo" snackbar. * With http://crrev.com/c/6555582, we are able to obtain the MotionEvent that triggered a click on an AppMenu item, so we can use it to differentiate between peripheral clicks and other clicks. * This CL passes the click-triggering MotionEvent from AppMenuItemViewBinder to MenuOrKeyboardActionController.onMenuOrKeyboardAction() where the actual click-handling logic lives. Similar work was done for the grid tab switcher: http://crrev.com/c/6521282. * The change involves a few interfaces. The code path is: AppMenuItemViewBinder -> [interface] AppMenuClickHandler.onItemClick() -> [impl] AppMenu.onItemClick() AppMenuHandlerImpl.onOptionsItemSelected() -> [interface] AppMenuDelegate.onOptionsItemSelected -> [impl] ChromeActivity.onOptionsItemSelected() [impl] CustomTabActivity.onOptionsItemSelected() [interface] MenuOrKeyboardActionController.onMenuOrKeyboardAction() [impl] ChromeTabbedActivity.onMenuOrKeyboardAction() [impl] ChromeActivity.onMenuOrKeyboardAction() [impl] BaseCustomTabActivity.onMenuOrKeyboardAction() [impl] CustomTabActivity.onMenuOrKeyboardAction() [impl] WebappActivity.onMenuOrKeyboardAction() This CL adds the click-triggering motion event as a parameter of the methods above. * To limit the scope of the CL, current interface methods are kept as default implementation. For example: ``` public interface AppMenuClickHandler { // Generally there's no need to override this method. default void onItemClick(/* current params */) { onItemClick( /* current params */, /* triggeringMotionEvent= */ null); } // Implementations should always implement this method. void onItemClick( /* currentParams */, MotionEvent triggeringMotionEvent); } ``` Bug: 375468032 Change-Id: I90019b2c02f54af6123280e4099c1b1dbcede585 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6557444 and https://source.chromium.org/chromium/chromium/src/+/01b2bf1ec0c43a1924269c22c07ff7a2cbccb6d7 Consolidate how MotionEvent info is passed for tab closure TL;DR: * This CL is a no-op; it only changes method signatures. * Reviewers: please follow the "Changes" section below to get a clearer view of the changes. Context: We needed info from the MotionEvent that triggered a click to differentiate between a peripheral click and other clicks so that tab closure behavior can be customized. For example, we don't want to show the undo snackbar when a tab is closed by a peripheral. Multiple UI surfaces have been updated so far, and we used two ways to pass MotionEvent info. (1) For a View that has OnClickListener: We attach an OnPeripheralClickListener to intercept MotionEvents from peripherals, then pass the raw MotionEvent to the click-handling logic. (2) For a ListView that relies on OnItemClickListener instead of adding OnClickListener to its individual child Views: We use TouchTrackingListView to watch (but not intercept) touch events and store the last relevant event as a state. Then, when OnItemClickListener is triggered, a child View can query that state to get motion info. Since MotionEvent will be recycled by Android framework, the state is in the form of a plain old data class containing motion info (ListViewTouchInfo). So far the UI surfaces are either (1) or (2) at compile time. However, there is at least one place that are both (1) and (2) at compile time. They become either (1) or (2) at runtime depending on the usage/configuration. TabListEditorMenu is one example. To better support such places, we need to consolidate how motion info is passed for (1) and (2) so that the click-handling methods can have a single signature: * Instead of: * handleClick(MotionEvent) // for (1) * handleClick(ListViewTouchInfo) // for (2) * It would be better to have: * handleClick(MotionEventInfo) // for both (1) and (2) Changes: * Move ListViewTouchInfo out of ListViewTouchTracker, to //browser_ui/util/motion/MotionEventInfo. We will use this class to pass motion info around for both (1) and (2). * Update OnPeripheralClickListener to pass the new MotionEventInfo object instead of a raw MotionEvent. * The rest of the CL is to fix method signatures for the two changes above. Note: Generally it's also safer to pass motion info as a data object instead of using a raw MotionEvent since the correctness of the latter requires the MotionEvent not to be recycled by the Android framework before it's read. Bug: 375468032 Change-Id: I19c0a468761403a7fa29d072e9b51ff73013dc5c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6565526 --- android/java/apk_for_test.flags | 6 +++++- .../chromium/chrome/browser/app/BraveActivity.java | 11 +++++++---- .../org/chromium/chrome/browser/BytecodeTest.java | 10 ++++++++++ 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/android/java/apk_for_test.flags b/android/java/apk_for_test.flags index 8cb7dbbfaf8..e5121a5dd2c 100644 --- a/android/java/apk_for_test.flags +++ b/android/java/apk_for_test.flags @@ -358,7 +358,11 @@ -keep class org.chromium.chrome.browser.search_engines.settings.BraveBaseSearchEngineAdapter --keep class org.chromium.chrome.browser.ChromeTabbedActivity +-keep class org.chromium.components.browser_ui.util.motion.MotionEventInfo + +-keep class org.chromium.chrome.browser.ChromeTabbedActivity { + boolean onMenuOrKeyboardAction(int, boolean, org.chromium.components.browser_ui.util.motion.MotionEventInfo); +} -keep class org.chromium.chrome.browser.app.BraveActivity diff --git a/android/java/org/chromium/chrome/browser/app/BraveActivity.java b/android/java/org/chromium/chrome/browser/app/BraveActivity.java index 28179ef755e..b6d0951c606 100644 --- a/android/java/org/chromium/chrome/browser/app/BraveActivity.java +++ b/android/java/org/chromium/chrome/browser/app/BraveActivity.java @@ -209,6 +209,7 @@ import org.chromium.chrome.browser.vpn.utils.BraveVpnUtils; import org.chromium.chrome.browser.vpn.wireguard.WireguardConfigUtils; import org.chromium.chrome.browser.widget.quickactionsearchandbookmark.promo.SearchWidgetPromoPanel; import org.chromium.components.browser_ui.settings.SettingsNavigation; +import org.chromium.components.browser_ui.util.motion.MotionEventInfo; import org.chromium.components.embedder_support.util.UrlConstants; import org.chromium.components.embedder_support.util.UrlUtilities; import org.chromium.components.prefs.PrefChangeRegistrar; @@ -406,7 +407,8 @@ public abstract class BraveActivity extends ChromeActivity } @Override - public boolean onMenuOrKeyboardAction(int id, boolean fromMenu) { + public boolean onMenuOrKeyboardAction( + int id, boolean fromMenu, @Nullable MotionEventInfo triggeringMotion) { final Tab currentTab = getActivityTab(); // Handle items replaced by Brave. if (id == R.id.info_menu_id && currentTab != null) { @@ -417,7 +419,7 @@ public abstract class BraveActivity extends ChromeActivity setComesFromNewTab(true); } - if (super.onMenuOrKeyboardAction(id, fromMenu)) { + if (super.onMenuOrKeyboardAction(id, fromMenu, triggeringMotion)) { return true; } @@ -748,7 +750,8 @@ public abstract class BraveActivity extends ChromeActivity } @Override - public boolean onOptionsItemSelected(int itemId, @Nullable Bundle menuItemData) { + public boolean onOptionsItemSelected( + int itemId, @Nullable Bundle menuItemData, @Nullable MotionEventInfo triggeringMotion) { if (itemId == R.id.new_tab_menu_id) { LayoutManagerChrome layoutManager = (LayoutManagerChrome) @@ -765,7 +768,7 @@ public abstract class BraveActivity extends ChromeActivity ((BraveToolbarManager) getToolbarManager()).openHomepage(); } } - return super.onOptionsItemSelected(itemId, menuItemData); + return super.onOptionsItemSelected(itemId, menuItemData, triggeringMotion); } @Override diff --git a/android/javatests/org/chromium/chrome/browser/BytecodeTest.java b/android/javatests/org/chromium/chrome/browser/BytecodeTest.java index 08467db6697..5aa0fe9c78d 100644 --- a/android/javatests/org/chromium/chrome/browser/BytecodeTest.java +++ b/android/javatests/org/chromium/chrome/browser/BytecodeTest.java @@ -159,6 +159,7 @@ import org.chromium.components.browser_ui.site_settings.SiteSettingsCategory; import org.chromium.components.browser_ui.site_settings.Website; import org.chromium.components.browser_ui.site_settings.WebsiteAddress; import org.chromium.components.browser_ui.site_settings.WebsitePermissionsFetcher.WebsitePermissionsType; +import org.chromium.components.browser_ui.util.motion.MotionEventInfo; import org.chromium.components.browser_ui.widget.MenuOrKeyboardActionController; import org.chromium.components.browser_ui.widget.RadioButtonWithDescription; import org.chromium.components.browser_ui.widget.RadioButtonWithEditText; @@ -978,6 +979,15 @@ public class BytecodeTest { "getBrowserServicesThemeColorProvider", MethodModifier.REGULAR, BrowserServicesThemeColorProvider.class)); + Assert.assertTrue( + methodExists( + "org/chromium/chrome/browser/ChromeTabbedActivity", + "onMenuOrKeyboardAction", + MethodModifier.REGULAR, + boolean.class, + int.class, + boolean.class, + MotionEventInfo.class)); } @Test