Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1349563003:
Implement Android M text process action in ActionMode (Closed)
|
Created:
5 years, 3 months ago by hush (inactive) Modified:
5 years, 2 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, boliu Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement Android M text process action in ActionMode
When there is any app that handles "ACTION_PROCESS_TEXT" intent,
"Translate" menu item will be shown.
This is feature is only supported on Android M and above, because
ACTION_PROCESS_TEXT is in M.
BUG=521027
Committed: https://crrev.com/bed1a7022badf2d5bb83c447fa8ae001b22b8c44
Cr-Commit-Position: refs/heads/master@{#350959}
Patch Set 1 : #
Total comments: 1
Patch Set 2 : #
Total comments: 3
Patch Set 3 : remove "translate" when necessary #
Total comments: 2
Patch Set 4 : Get all processing text action menus #Patch Set 5 : #
Total comments: 12
Patch Set 6 : comments #
Total comments: 6
Patch Set 7 : fix indents #
Total comments: 8
Messages
Total messages: 43 (15 generated)
Patchset #1 (id:1) has been deleted
hush@chromium.org changed reviewers: + jdduke@chromium.org
PTAL! Chrome should be able to replace the select text with the translated text in "onActivityResult". I can do that in a follow up CL.
https://codereview.chromium.org/1349563003/diff/20001/content/public/android/... File content/public/android/java/res/menu/select_action_menu.xml (right): https://codereview.chromium.org/1349563003/diff/20001/content/public/android/... content/public/android/java/res/menu/select_action_menu.xml:46: android:id="@+id/select_action_menu_translate" the reason why I put translate before search is to make sure translate shows up in the first level menu items in landscape mode...
hush@chromium.org changed reviewers: + newt@chromium.org
+newt
https://codereview.chromium.org/1349563003/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1349563003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2105: final String query = sanitizeQuery(getSelectedText(), MAX_SEARCH_QUERY_LENGTH); It's not necessarily translate though, right? Don't we have to poll the PROCCESS_TEXT handlers to determine what actions are available? Something like what is done with Editor and ProcessTextIntentActionsHandler? Or is that not possible? https://codereview.chromium.org/1349563003/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java (right): https://codereview.chromium.org/1349563003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java:201: menu.removeItem(R.id.select_action_menu_cut); I imagine we'd want to remove translate here as well.
https://codereview.chromium.org/1349563003/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1349563003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2105: final String query = sanitizeQuery(getSelectedText(), MAX_SEARCH_QUERY_LENGTH); On 2015/09/16 22:31:55, jdduke (OOO until Sept 21) wrote: > It's not necessarily translate though, right? Don't we have to poll the > PROCCESS_TEXT handlers to determine what actions are available? Something like > what is done with Editor and ProcessTextIntentActionsHandler? Or is that not > possible? Possible, I guess. I used to think "process text action" is just a fancy name for "translate". I will check with the Android guys about that.
On 2015/09/16 23:23:40, hush wrote: > https://codereview.chromium.org/1349563003/diff/40001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > https://codereview.chromium.org/1349563003/diff/40001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2105: > final String query = sanitizeQuery(getSelectedText(), MAX_SEARCH_QUERY_LENGTH); > On 2015/09/16 22:31:55, jdduke (OOO until Sept 21) wrote: > > It's not necessarily translate though, right? Don't we have to poll the > > PROCCESS_TEXT handlers to determine what actions are available? Something like > > what is done with Editor and ProcessTextIntentActionsHandler? Or is that not > > possible? > > Possible, I guess. I used to think "process text action" is just a fancy name > for "translate". I will check with the Android guys about that. Yes, pretty sure this is supposed to be generic and not just for translate.
torne@chromium.org changed reviewers: + torne@chromium.org
https://codereview.chromium.org/1349563003/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1349563003/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2113: i.putExtra(Intent.EXTRA_PROCESS_TEXT_READONLY, !isSelectionEditable()); Since we can't currently replace the text we should probably always claim to be readonly?
I'm not an owner in content, nor am I familiar with this code. I think someone else should take a look.
Patchset #4 (id:80001) has been deleted
hush@chromium.org changed reviewers: + tedchoc@chromium.org - jdduke@chromium.org, newt@chromium.org
Hi Ted, can you take a look? Thanks!
https://codereview.chromium.org/1349563003/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1349563003/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2113: i.putExtra(Intent.EXTRA_PROCESS_TEXT_READONLY, !isSelectionEditable()); On 2015/09/17 10:26:17, Torne wrote: > Since we can't currently replace the text we should probably always claim to be > readonly? done
https://codereview.chromium.org/1349563003/diff/120001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1349563003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2110: for (ResolveInfo resolveInfo : getSupportedTextProcessingActivities()) { I would iterate over the list using the for (int i = 0; ...) format. It removes the i tracking outside of it and avoids the iterator allocation. https://codereview.chromium.org/1349563003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2112: getLabel(resolveInfo)) this line needs to be indented 4 more https://codereview.chromium.org/1349563003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2120: if (Build.VERSION.SDK_INT < Build.VERSION_CODES.M) return; we should probably assert false here since it would imply a coding error. https://codereview.chromium.org/1349563003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2156: if (Build.VERSION.SDK_INT < Build.VERSION_CODES.M) { I would just inline this function above to above this check https://codereview.chromium.org/1349563003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2177: private CharSequence getLabel(ResolveInfo resolveInfo) { seems unnecessary, I would just inline above. Does this cause a strict mode violation? Just curious. https://codereview.chromium.org/1349563003/diff/120001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java (right): https://codereview.chromium.org/1349563003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java:64: void initializeTextProcessingMenu(Menu menu); To me, this function doesn't seem like it belongs on the ActionHandler but should just live within this class. If we want the embedder to have the ability to toggle support for this, I think we should have supportsTextProcessIntents as a function to keep it consistent with the isSelectionEditable as below. But for now, I would just include it all in this file.
https://codereview.chromium.org/1349563003/diff/120001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1349563003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2110: for (ResolveInfo resolveInfo : getSupportedTextProcessingActivities()) { On 2015/09/17 20:34:57, Ted C wrote: > I would iterate over the list using the for (int i = 0; ...) format. It removes > the i tracking outside of it and avoids the iterator allocation. Done. https://codereview.chromium.org/1349563003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2112: getLabel(resolveInfo)) On 2015/09/17 20:34:57, Ted C wrote: > this line needs to be indented 4 more Done. https://codereview.chromium.org/1349563003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2120: if (Build.VERSION.SDK_INT < Build.VERSION_CODES.M) return; On 2015/09/17 20:34:57, Ted C wrote: > we should probably assert false here since it would imply a coding error. Done. https://codereview.chromium.org/1349563003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2156: if (Build.VERSION.SDK_INT < Build.VERSION_CODES.M) { On 2015/09/17 20:34:57, Ted C wrote: > I would just inline this function above to above this check Done. https://codereview.chromium.org/1349563003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2177: private CharSequence getLabel(ResolveInfo resolveInfo) { On 2015/09/17 20:34:57, Ted C wrote: > seems unnecessary, I would just inline above. > > Does this cause a strict mode violation? Just curious. Done. And it does not cause any strict mode violations. https://codereview.chromium.org/1349563003/diff/120001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java (right): https://codereview.chromium.org/1349563003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java:64: void initializeTextProcessingMenu(Menu menu); On 2015/09/17 20:34:57, Ted C wrote: > To me, this function doesn't seem like it belongs on the ActionHandler but > should just live within this class. > > If we want the embedder to have the ability to toggle support for this, I think > we should have supportsTextProcessIntents as a function to keep it consistent > with the isSelectionEditable as below. But for now, I would just include it all > in this file. Okay. I just moved the related logic from contentviewcore to this class. I don't add supportsTextProcessIntents() yet. I haven't seen any use case that yet. People can add it easily when it is necessary.
lgtm https://codereview.chromium.org/1349563003/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java (right): https://codereview.chromium.org/1349563003/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java:269: public void initializeTextProcessingMenu(Menu menu) { private? https://codereview.chromium.org/1349563003/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java:279: .setIntent(createProcessTextIntentForResolveInfo(resolveInfo)) +4 indenting for these lines https://codereview.chromium.org/1349563003/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java:294: .putExtra(Intent.EXTRA_PROCESS_TEXT_READONLY, true) same indenting nit as above
https://codereview.chromium.org/1349563003/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java (right): https://codereview.chromium.org/1349563003/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java:269: public void initializeTextProcessingMenu(Menu menu) { On 2015/09/17 22:07:36, Ted C wrote: > private? Done. https://codereview.chromium.org/1349563003/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java:279: .setIntent(createProcessTextIntentForResolveInfo(resolveInfo)) On 2015/09/17 22:07:36, Ted C wrote: > +4 indenting for these lines Done. https://codereview.chromium.org/1349563003/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java:294: .putExtra(Intent.EXTRA_PROCESS_TEXT_READONLY, true) On 2015/09/17 22:07:36, Ted C wrote: > same indenting nit as above Done.
The CQ bit was checked by hush@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/1349563003/#ps160001 (title: "fix indents")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349563003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349563003/160001
On 2015/09/17 22:20:04, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1349563003/160001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1349563003/160001 I wouldn't mind looking at this before you commit, can you wait until I'm back Monday? Is it time sensitive?
The CQ bit was unchecked by hush@chromium.org
On 2015/09/17 22:40:04, jdduke (OOO until Sept 21) wrote: > On 2015/09/17 22:20:04, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/patch-status/1349563003/160001 > > View timeline at > > https://chromium-cq-status.appspot.com/patch-timeline/1349563003/160001 > > I wouldn't mind looking at this before you commit, can you wait until I'm back > Monday? Is it time sensitive? Sure, it's not time sensitive.
On 2015/09/17 22:51:19, hush wrote: > On 2015/09/17 22:40:04, jdduke (OOO until Sept 21) wrote: > > On 2015/09/17 22:20:04, commit-bot: I haz the power wrote: > > > CQ is trying da patch. Follow status at > > > https://chromium-cq-status.appspot.com/patch-status/1349563003/160001 > > > View timeline at > > > https://chromium-cq-status.appspot.com/patch-timeline/1349563003/160001 > > > > I wouldn't mind looking at this before you commit, can you wait until I'm back > > Monday? Is it time sensitive? > > Sure, it's not time sensitive. Hi Jared, have you taken a look yet?
jdduke@chromium.org changed reviewers: + jdduke@chromium.org
lgtm assuming it works and Ted's happy :) https://codereview.chromium.org/1349563003/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java (right): https://codereview.chromium.org/1349563003/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java:273: List<ResolveInfo> supportedActivities = How expensive is this query? Is it worth early-outing for insertion/password fields? https://codereview.chromium.org/1349563003/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java:292: // TODO(hush crbug.com/521027): should be !isSelectionEditable(), Please move the |true| boolean arg out and assign it a name before passing in, with the comment there. Also, not sure I've seen the crbug in the TODO() portion, maybe put that at the end, TODO(hush): *, crbug.com/*.
https://codereview.chromium.org/1349563003/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java (right): https://codereview.chromium.org/1349563003/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java:270: if (Build.VERSION.SDK_INT < Build.VERSION_CODES.M) return; Oh, did we decide if we wanted this only for floating ActionModes? Because we can get both types on M.
https://codereview.chromium.org/1349563003/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java (right): https://codereview.chromium.org/1349563003/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java:270: if (Build.VERSION.SDK_INT < Build.VERSION_CODES.M) return; On 2015/09/24 20:05:14, jdduke wrote: > Oh, did we decide if we wanted this only for floating ActionModes? Because we > can get both types on M. I chatted with the feature owner in Android. He is not sure himself. So I just made a test app myself with customized text view in it. Both types of ActionModes should get "Text processing" actions for TextView. So let's just do that WebView too (which is what's done right now) https://codereview.chromium.org/1349563003/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java:273: List<ResolveInfo> supportedActivities = On 2015/09/24 20:01:21, jdduke wrote: > How expensive is this query? Is it worth early-outing for insertion/password > fields? Not sure how expensive it is but the function is pretty complicated. https://cs.corp.google.com/#android/frameworks/base/services/core/java/com/an... I will just put initializeTextProcessingMenu in the end of createActionMenu https://codereview.chromium.org/1349563003/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java:292: // TODO(hush crbug.com/521027): should be !isSelectionEditable(), On 2015/09/24 20:01:21, jdduke wrote: > Please move the |true| boolean arg out and assign it a name before passing in, > with the comment there. Also, not sure I've seen the crbug in the TODO() > portion, maybe put that at the end, TODO(hush): *, crbug.com/*. Done.
The CQ bit was checked by hush@chromium.org
The CQ bit was unchecked by hush@chromium.org
The CQ bit was checked by hush@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349563003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349563003/160001
https://codereview.chromium.org/1349563003/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java (right): https://codereview.chromium.org/1349563003/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java:273: List<ResolveInfo> supportedActivities = On 2015/09/25 23:05:30, hush wrote: > On 2015/09/24 20:01:21, jdduke wrote: > > How expensive is this query? Is it worth early-outing for insertion/password > > fields? > Not sure how expensive it is but the function is pretty complicated. > https://cs.corp.google.com/#android/frameworks/base/services/core/java/com/an... > > I will just put initializeTextProcessingMenu in the end of createActionMenu Sounds good, so we'll just not call this function if it's a password or insertion type?
https://codereview.chromium.org/1349563003/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java (right): https://codereview.chromium.org/1349563003/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java:273: List<ResolveInfo> supportedActivities = On 2015/09/25 23:36:00, jdduke wrote: > On 2015/09/25 23:05:30, hush wrote: > > On 2015/09/24 20:01:21, jdduke wrote: > > > How expensive is this query? Is it worth early-outing for insertion/password > > > fields? > > Not sure how expensive it is but the function is pretty complicated. > > > https://cs.corp.google.com/#android/frameworks/base/services/core/java/com/an... > > > > I will just put initializeTextProcessingMenu in the end of createActionMenu > > Sounds good, so we'll just not call this function if it's a password or > insertion type? Yes.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hush@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1349563003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1349563003/160001
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/bed1a7022badf2d5bb83c447fa8ae001b22b8c44 Cr-Commit-Position: refs/heads/master@{#350959} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
