Android: Simplify select action mode configuration |
|||||
Issue descriptionCurrently Select Action Mode has some confusing flows to override behavior for: 1) content- and selection-specific: Whether we are able to insert, the selection is editable, we are dealing with a password, or are in incognito mode I believe amarlp's work in https://codereview.chromium.org/2089933002/ would allow this to be configured at creation time rather than with all these callbacks to the WebContents/CVC/IME at runtime. 2) WebView-specific: Here it also seems a bit weird to go back from WebActionModeCallback to ActionHandler which is implemented inside content (ContentViewCore) to then go back to ContentViewClient to let the embedder override behavior (rather than letting the embedder override or configure the WebActionMode(Callback) accordingly in the first place). The current behavioral overrides are as follows: a) For Chrome 'process text' intents are forwarded to ActivityWindowAndroid where we start the extent from the window's activity. In WebView this gets forwarded to AwContents where it calls the hidden View method startActivityForResult(). Also see https://codereview.chromium.org/1409443002/ This would be nicer to override through WindowAndroid which is also overriden for WebView nowadays, see AwContents. However, WebView will only create an ActivityWindowAndroid if it has an activity context. If not (application context or service) then it will create WindowAndroidWrapper, but the base class does not implement showCancelableIntent() since there is no Activity. It's not clear how that works today in the existing code when View#getContext() does not contain an activity context since the documentation for startActivity() says you'd need to specify FLAG_ACTIVITY_NEW_TASK if you are starting an activity from a non-activity context since there'd be no existing task to associate it with. Maybe it currently throws and exception so it might actually be better to silently fail it in our code instead. This part is essentially the ContentViewClient.doesPerformProcessText()/startProcessTextIntent() override. b) Whether we want to hide the 'web search' and 'share' functionality for WebView's configured that way. This works through isSelectActionModeAllowed() and is based on AwSettings. c) Override web search behavior. Here it's actually the other way around: WebView does not override the default behavior but Chrome does so that the search happens in a new Tab while otherwise it creates an intent. The latter is the default behavior in ContentViewCore and it calls getContenxt().startActivity() explicitly with FLAG_ACTIVITY_NEW_TASK. On a related note it is weird in Chrome here that we have contextual search popping up here with the bottom panel when you select, and at the same time still have the 'web search' option (in a new tab) in the action menu.
,
Jun 28 2016
hush: where is the action mode intent fired when webview doesn't have an activity? seem to work fine on gsa
,
Jun 28 2016
You're saying GSA does not create webview with an activity context? We set the intent that is supposed to be fired on the menu item. https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java?rcl=0&l=288 Then *I think* (I'll read some frameworks code to verify) the framework is responsible for firing them when they are clicked.
,
Jun 28 2016
FWIW, I recently was looking at adding another conditional entry in the ActionMode, and the current behavior/layering is quite limiting. In particular, I want to add an option only in Chrome, and I need to muck around in the content layer to make that happen. Whatever approach we take, let's make sure the embedder can easily add/remove options w/o having to change content.
,
Jun 28 2016
correcting myself in #3: framework does not fire intent when the menu items are clicked, we do. For Web Search and Share, the intents are fired with "FLAG_ACTIVITY_NEW_TASK" added, see https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java?rcl=0&l=2027 For processing text menu items, webview will start the activity using the wrapped context: https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/android_webview/AwContents.java?rcl=0&l=2326 The wrapped context automatically adds the FLAG_ACTIVITY_NEW_TASK flag when the context is not an activity: https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/android_webview/ResourcesContextWrapperFactory.java?rcl=0&l=115 So, we're covered in all grounds.
,
Jun 28 2016
Re #4: WebView-using also need to add items to the floating action popup. GMail wants to do this for their custom rich-text formatting menu. It sounded like there was already a way that they found to do this using existing APIs.
,
Jun 28 2016
I suppose they can override startActionMode, take whatever chromium returns, and append their own menu item to it
,
Jun 28 2016
Re #5: Thanks for verifying all that. Do you think we could reroute WebView through the WindowAndroid function also for firing this? WindowAndroidWrapper could just implement this accordingly inside AwContents.
,
Jun 28 2016
Re #6: The only really dynamic portion I've seen is what is created under the 'process text' group (this is not a menu item, but a group) where we ask the package manager for all activities that process this intent and then add them to the menu, see WebActionModeCallback.initializeTextProcessingMenu. With amaralp's work in progress he is already adding a call to the embedder through WebContents(View)Delegate (actually that interface already exists). That's one potential place for adding extra stuff and to customize the menu.
,
Jun 28 2016
re #8: yeah I think it'll work for web search, share and text processing in webview.
,
Jun 28 2016
> Re #5: Thanks for verifying all that. Do you think we could reroute WebView through the WindowAndroid function also for firing this? WindowAndroidWrapper could just implement this accordingly inside AwContents. WindowAndroidWrapper is a wrapper (for lifetime guarantees). It's not a subclass of WindowAndroid..
,
Jun 28 2016
Is it ok to subclass though for handling intents?
,
Jun 28 2016
Can we just do it in WindowAndroid? WindowAndroid should really be a webview-only thing. Chrome always creates ActivityWindowAndroid. Looks like chromecast creates WindowAndroid by mistake.
,
Jun 29 2016
> Can we just do it in WindowAndroid? Yea that should work too since it now just calls mContext.startActivity(intent) (did that just change?). It used to call into the View I thought and it wouldn't be good for WindowAndroid to reference the View. > WindowAndroid should really be a webview-only thing. Looks like chromecast creates WindowAndroid by mistake. No WebView-specifics in content/ please though either.
,
Aug 24 2016
,
Oct 13 2016
Let me handle this. Select action mode behavior is largely implemented in ContentViewCore, and embbeder is supposed to put impl-specific logic in ContentViewClient. With ContentViewClient going away, it won't work anymore - and it's a good opportunity to make overall flow simpler. The approach I have in mind is to let embedder provide its own WebActionMode (+ ActionHandler). I'll factor all the commonalities out from ContentViewCore which will go away in the end too, and define a base class of the action handler for embedders to make it easy to define the impl-specific behavior. This is basically as good as moving the action mode-related logic that resides now in ContentViewClient to WebActionModeCallback.ActionHandler.
,
Oct 18 2016
,
Nov 10 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1eef196331dcff4093afabf4d4b5507fd84dfa94 commit 1eef196331dcff4093afabf4d4b5507fd84dfa94 Author: jinsukkim <jinsukkim@chromium.org> Date: Thu Nov 10 18:39:37 2016 Let embedder provide select action mode This CL tries to simplify selection action mode configuration being done through ContentViewClient that will be gone after refactoring. Now Embedders provide with a customized ActionMode.Callback. This effectively moves the relevant implementation details from ContentViewClient to the callback. SelectionPopupController pulls out all the select action mode-related methods and variables from ContentViewCore and encapsulates them. This also helps break up ContentViewCore. BUG= 623783 Review-Url: https://codereview.chromium.org/2407303005 Cr-Commit-Position: refs/heads/master@{#431299} [modify] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/android_webview/BUILD.gn [add] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/android_webview/java/src/org/chromium/android_webview/AwActionModeCallback.java [modify] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java [modify] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/android_webview/java/src/org/chromium/android_webview/AwContents.java [modify] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/android_webview/javatests/src/org/chromium/android_webview/test/PopupWindowTest.java [add] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/chrome/android/java/src/org/chromium/chrome/browser/ChromeActionModeCallback.java [modify] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/chrome/android/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java [modify] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java [modify] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/chrome/android/java_sources.gni [modify] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java [modify] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchTapEventTest.java [modify] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/content/browser/android/content_view_core_impl.cc [modify] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/content/browser/android/content_view_core_impl.h [modify] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/content/browser/web_contents/web_contents_android.cc [modify] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/content/browser/web_contents/web_contents_android.h [modify] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/content/public/android/BUILD.gn [modify] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java [modify] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java [add] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/content/public/android/java/src/org/chromium/content/browser/FloatingActionModeCallback.java [delete] https://crrev.com/af3a4aa28c7f9a6fd0c944fd0f046a61bce0aa03/content/public/android/java/src/org/chromium/content/browser/FloatingWebActionModeCallback.java [add] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java [delete] https://crrev.com/af3a4aa28c7f9a6fd0c944fd0f046a61bce0aa03/content/public/android/java/src/org/chromium/content/browser/WebActionMode.java [delete] https://crrev.com/af3a4aa28c7f9a6fd0c944fd0f046a61bce0aa03/content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java [modify] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/content/public/android/java/src/org/chromium/content/browser/input/FloatingPastePopupMenu.java [modify] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java [add] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/content/public/android/java/src/org/chromium/content_public/browser/ActionModeCallbackHelper.java [modify] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/content/public/android/java/src/org/chromium/content_public/browser/WebContents.java [modify] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreSelectionTest.java [modify] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/content/public/android/javatests/src/org/chromium/content/browser/PopupZoomerTest.java [modify] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java [modify] https://crrev.com/1eef196331dcff4093afabf4d4b5507fd84dfa94/content/shell/android/java/src/org/chromium/content_shell/Shell.java
,
Nov 23 2016
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by siev...@chromium.org
, Jun 28 2016