Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1409443002:
Make Chrome and WebView replace the selected text with the processed text. (Closed)
|
Created:
5 years, 2 months ago by hush (inactive) Modified:
5 years, 2 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake Chrome and WebView replace the selected text with the processed text.
This CL uses WindowAndroid to send intents and receive results for Chrome. It
is not possible to do the same for WebView because WebView is not by itself an
application.
WebView needs to use View#startActivityForResult to send intents and use
View#onActivityResult to receive the process text. Both of these View APIs are
in M.
BUG=521027
Committed: https://crrev.com/bc5bc8853ce851bc323a2d4fbddb25abacd0aafe
Cr-Commit-Position: refs/heads/master@{#355858}
Patch Set 1 : #Patch Set 2 : #
Total comments: 17
Patch Set 3 : rebase #Patch Set 4 : address review #
Total comments: 1
Patch Set 5 : address Bo's comments #
Total comments: 13
Patch Set 6 : address some comments #Patch Set 7 : javadoc #
Total comments: 6
Patch Set 8 : #
Total comments: 1
Patch Set 9 : #Patch Set 10 : moved comments around #Messages
Total messages: 40 (15 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
hush@chromium.org changed reviewers: + jdduke@chromium.org
Hi Jared, can you take a look? WebView just exposed 2 methods to be able to send intents from WebView java object, and receive the result in this CL https://codereview.chromium.org/1399613002/ With these 2 methods, webview should be able to replace the selected text with the translated text.
Description was changed from ========== Make Chrome and WebView replace the selected text with the processed text. This CL uses WindowAndroid to send intents and receive results for Chrome. It is not possible to do the same for WebView because WebView is not by itself an application. WebView needs to use View#startActivityForResult to send intents and use View#onActivityResult to receive the process text. Both of these View APIs are in M. BUG=521027 ========== to ========== Make Chrome and WebView replace the selected text with the processed text. This CL uses WindowAndroid to send intents and receive results for Chrome. It is not possible to do the same for WebView because WebView is not by itself an application. WebView needs to use View#startActivityForResult to send intents and use View#onActivityResult to receive the process text. Both of these View APIs are in M. BUG=521027 ==========
hush@chromium.org changed reviewers: + boliu@chromium.org
On 2015/10/20 23:36:37, hush wrote: > Hi Jared, > can you take a look? > WebView just exposed 2 methods to be able to send intents from WebView java > object, and receive the result in this CL > https://codereview.chromium.org/1399613002/ > > With these 2 methods, webview should be able to replace the selected text with > the translated text. +boliu for android_webview/
Couple questions. https://chromiumcodereview.appspot.com/1409443002/diff/80001/content/public/a... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (left): https://chromiumcodereview.appspot.com/1409443002/diff/80001/content/public/a... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2094: getContext().startActivity(intent); So this just failed with an error log before on WebView? https://chromiumcodereview.appspot.com/1409443002/diff/80001/content/public/a... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://chromiumcodereview.appspot.com/1409443002/diff/80001/content/public/a... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2097: getWindowAndroid().showIntent(intent, ContentViewCore.this, null); I have a slight preference for either creating a new callback here (instead of using |this|), or lazily constructing a member callback. Are there cases where we would want/need to cancel the outstanding callback, e.g., if the response is delayed or something like that? I know WindowAndroid affords that capability. Might be useful to cancel the intent when the ActionMode gets destroyed, in case it's dismissed for any other reason while still waiting on the response. https://chromiumcodereview.appspot.com/1409443002/diff/80001/content/public/a... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3055: onReceivedProcessTextResult(resultCode, data); Is there any simple way to validate here that this callback is only fired for the EXTRA_PROCESS_TEXT intent? I guess having a dedicated callback for that purpose would help that assumption. https://chromiumcodereview.appspot.com/1409443002/diff/80001/content/public/a... File content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java (right): https://chromiumcodereview.appspot.com/1409443002/diff/80001/content/public/a... content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java:236: // here. ActionMode will be dismissed after the activity result is What kind of guarantees do we have that we'll get a result? If we don't clear it here, could the user conceivable repeatedly trigger the button before we get an async response?
https://codereview.chromium.org/1409443002/diff/80001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java (right): https://codereview.chromium.org/1409443002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:28: /* package */ static final int PROCESS_TEXT_REQUEST_CODE = 100; So... 100 won't collide with anything or what..? Seems this number should be saved somewhere closer to android, rather than in chromium code https://codereview.chromium.org/1409443002/diff/80001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1409443002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2178: public void startActivityForResult(Context context, Intent intent, int requestCode) { this should be package visible? https://codereview.chromium.org/1409443002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2191: if (requestCode == AwContentViewClient.PROCESS_TEXT_REQUEST_CODE) { Does anything else call startActivityForResult? If not, can keep the static code in this class, and private
https://codereview.chromium.org/1409443002/diff/80001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java (right): https://codereview.chromium.org/1409443002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:28: /* package */ static final int PROCESS_TEXT_REQUEST_CODE = 100; On 2015/10/21 00:35:26, boliu wrote: > So... 100 won't collide with anything or what..? > > Seems this number should be saved somewhere closer to android, rather than in > chromium code We only have to make sure the request code is unique within webview (that is, one startActivityForResult at a time, and WebView always receives the activity result for the previous "startActivityForResult" before the next "startActivityForResult" is called.) This is basically the same as the assumption in TextView.java https://cs.corp.google.com/#android/frameworks/base/core/java/android/widget/... The request code does not have to be globally unique because it is confined by "who" https://cs.corp.google.com/#android/frameworks/base/core/java/android/view/Vi... https://codereview.chromium.org/1409443002/diff/80001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1409443002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2178: public void startActivityForResult(Context context, Intent intent, int requestCode) { On 2015/10/21 00:35:27, boliu wrote: > this should be package visible? Yes. It can. (may be changed in the future depending on dgn@ implements his feature) https://codereview.chromium.org/1409443002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2191: if (requestCode == AwContentViewClient.PROCESS_TEXT_REQUEST_CODE) { On 2015/10/21 00:35:27, boliu wrote: > Does anything else call startActivityForResult? If not, can keep the static code > in this class, and private Something else may call startActivityForResult too, (I heard from Selim, dgn@ is developing a feature that uses it). So startActivityForResult is not only for text processing. That's why I put PROCESS_TEXT_REQUEST_CODE into AwContentViewClient. https://codereview.chromium.org/1409443002/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (left): https://codereview.chromium.org/1409443002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2094: getContext().startActivity(intent); On 2015/10/21 00:27:29, jdduke wrote: > So this just failed with an error log before on WebView? No, this works on WebView before, except that WebView won't be able to receive the "translated" text. The new code "startProcessTextIntent" will call View#startActivityForResult instead. https://codereview.chromium.org/1409443002/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1409443002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2097: getWindowAndroid().showIntent(intent, ContentViewCore.this, null); On 2015/10/21 00:27:29, jdduke wrote: > I have a slight preference for either creating a new callback here (instead of > using |this|), or lazily constructing a member callback. > > Are there cases where we would want/need to cancel the outstanding callback, > e.g., if the response is delayed or something like that? I know WindowAndroid > affords that capability. I don't know any case where we need to cancel the outstanding callback at the moment. At least that's not a requirement of this feature in M, and their example implementation in M with Editor.java and TextView.java does not handle such cases. We can move along and see if people complain about those cases if they do happen at all? > Might be useful to cancel the intent when the > ActionMode gets destroyed, in case it's dismissed for any other reason while > still waiting on the response. WindowAndroid code path can only used by Chrome. To do this, I can just check if ActionMode is destroyed or not in CVC#onReceivedProcessTextResult. In that way it benefits both Chrome and WebView. https://codereview.chromium.org/1409443002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3055: onReceivedProcessTextResult(resultCode, data); On 2015/10/21 00:27:29, jdduke wrote: > Is there any simple way to validate here that this callback is only fired for > the EXTRA_PROCESS_TEXT intent? I guess having a dedicated callback for that > purpose would help that assumption. At the moment, the callback object itself is mapped to the requestCode. The callback object is just CVC object. I wrote this code with the assumption that nobody else uses CVC to fire an intent through WindowAndroid. I will just create a dedicated callback object, because that's better. https://codereview.chromium.org/1409443002/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java (right): https://codereview.chromium.org/1409443002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java:236: // here. ActionMode will be dismissed after the activity result is On 2015/10/21 00:27:29, jdduke wrote: > What kind of guarantees do we have that we'll get a result? If we don't clear it > here, could the user conceivable repeatedly trigger the button before we get an > async response? I think WebView always gets a result, from looking at the documentation here: http://developer.android.com/training/basics/intents/filters.html Even if the activity does not call setResult(), the result will be "RESULT_CANCELED" by default. I also have tried crashing the receiving activity upon any "PROCESS_TEXT" intent. In this case, the parent activity (that is the WebView app activity) will also be killed by activity manager. So Android OS has made sure no matter what happens, you will receive a result, or crash. I think it will be an Android bug, if you fire an intent for result and never get it. The user cannot tap on multiple action mode buttons because the action bar is automatically hidden by Android OS when the activity goes to background (when the translation activity is in the foreground). In fact, WebView just needs to reach feature parity with the rest of Android. I just have to mimic the behavior of TextView, which is used by gmail's compose text activity. The action mode bar is NOT dismissed until the user replaces the selected text with the translated text. If the user just closes the translation activity, the selection and the action mode bar should remain on the original gmail activity.
Patchset #3 (id:100001) has been deleted
https://codereview.chromium.org/1409443002/diff/80001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1409443002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2191: if (requestCode == AwContentViewClient.PROCESS_TEXT_REQUEST_CODE) { On 2015/10/21 23:43:20, hush wrote: > On 2015/10/21 00:35:27, boliu wrote: > > Does anything else call startActivityForResult? If not, can keep the static > code > > in this class, and private > > Something else may call startActivityForResult too, (I heard from Selim, dgn@ is > developing a feature that uses it). So startActivityForResult is not only for > text processing. That's why I put PROCESS_TEXT_REQUEST_CODE into > AwContentViewClient. Ok, keep PROCESS_TEXT_REQUEST_CODE private to AwContents then, and expose startProcessTextIntent method to AwContentsClient? So then "send" and "receive" live next to each other. Spreading a "unique constant" across files should be avoided when possible.
https://codereview.chromium.org/1409443002/diff/80001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java (right): https://codereview.chromium.org/1409443002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:28: /* package */ static final int PROCESS_TEXT_REQUEST_CODE = 100; On 2015/10/21 23:43:20, hush wrote: > On 2015/10/21 00:35:26, boliu wrote: > > So... 100 won't collide with anything or what..? > > > > Seems this number should be saved somewhere closer to android, rather than in > > chromium code > > We only have to make sure the request code is unique within webview (that is, > one startActivityForResult at a time, and WebView always receives the activity > result for the previous "startActivityForResult" before the next > "startActivityForResult" is called.) This is basically the same as the > assumption in TextView.java > https://cs.corp.google.com/#android/frameworks/base/core/java/android/widget/... > > The request code does not have to be globally unique because it is confined by > "who" > https://cs.corp.google.com/#android/frameworks/base/core/java/android/view/Vi... Ok, should have a comment about how "unique" this needs to be.
https://codereview.chromium.org/1409443002/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java (right): https://codereview.chromium.org/1409443002/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java:232: // ActionMode will only be dismissed if the user replaces the Sorry I made a mistake here. TextView (and text editors) in Android M does not dismiss the action mode even after the user replaces the selected text with the processed text. It is dismissed when you do "copy", "cut", "paste" though.
https://codereview.chromium.org/1409443002/diff/80001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1409443002/diff/80001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:2191: if (requestCode == AwContentViewClient.PROCESS_TEXT_REQUEST_CODE) { On 2015/10/21 23:48:00, boliu wrote: > On 2015/10/21 23:43:20, hush wrote: > > On 2015/10/21 00:35:27, boliu wrote: > > > Does anything else call startActivityForResult? If not, can keep the static > > code > > > in this class, and private > > > > Something else may call startActivityForResult too, (I heard from Selim, dgn@ > is > > developing a feature that uses it). So startActivityForResult is not only for > > text processing. That's why I put PROCESS_TEXT_REQUEST_CODE into > > AwContentViewClient. > > Ok, keep PROCESS_TEXT_REQUEST_CODE private to AwContents then, and expose > startProcessTextIntent method to AwContentsClient? > > So then "send" and "receive" live next to each other. Spreading a "unique > constant" across files should be avoided when possible. Done.
a_w lgtm https://codereview.chromium.org/1409443002/diff/160001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1409443002/diff/160001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:2235: } maybe log the else block as an error?
OK, as long as we're reasonably close to TextView behavior as far as callback guarantees I'm not too worried. https://codereview.chromium.org/1409443002/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java (right): https://codereview.chromium.org/1409443002/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:108: public boolean doesPerformProcessText() { Javadoc, preferably with a bit more context behind usage here. https://codereview.chromium.org/1409443002/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1409443002/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3058: public void onReceivedProcessTextResult(int resultCode, Intent data) { Javadoc, definitely need some description here of usage and args. Also, since the callback can outlast the CVC practical lifetime, we'll want to early return if |mWebContents == null|. I suppose mActionMode should always be null in that case, but we should be explicit. https://codereview.chromium.org/1409443002/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3059: if (mActionMode == null) return; Can you do a sanity check that this works OK with both Chrome/WebView? I believe we clear the selection (thus nulling mActionMode) when we lose explicit focus (in onFocusChanged), though we do preserve it onHide and onWindowFocusChanged. https://codereview.chromium.org/1409443002/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java (right): https://codereview.chromium.org/1409443002/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java:233: // TextView in Android M. Hmm, will calling |Replace| both replace the text and select it? Or does it replace it and switching to an insertion caret? If the former, we should be OK, if the latter, the transition from selection to insertion will implicitly clear the ActionMode.
https://codereview.chromium.org/1409443002/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java (right): https://codereview.chromium.org/1409443002/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java:233: // TextView in Android M. On 2015/10/22 00:43:56, jdduke wrote: > Hmm, will calling |Replace| both replace the text and select it? Or does it > replace it and switching to an insertion caret? If the former, we should be OK, > if the latter, the transition from selection to insertion will implicitly clear > the ActionMode. That is to say, either way I think we're OK here. https://codereview.chromium.org/1409443002/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java:294: || Build.VERSION.SDK_INT == Build.VERSION_CODES.M; Wait, what? What about Chrome though?
https://codereview.chromium.org/1409443002/diff/160001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/1409443002/diff/160001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContents.java:2235: } On 2015/10/22 00:22:05, boliu wrote: > maybe log the else block as an error? Done. https://codereview.chromium.org/1409443002/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java (right): https://codereview.chromium.org/1409443002/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:108: public boolean doesPerformProcessText() { On 2015/10/22 00:43:56, jdduke wrote: > Javadoc, preferably with a bit more context behind usage here. Done. https://codereview.chromium.org/1409443002/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1409443002/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3058: public void onReceivedProcessTextResult(int resultCode, Intent data) { On 2015/10/22 00:43:56, jdduke wrote: > Javadoc, definitely need some description here of usage and args. > > Also, since the callback can outlast the CVC practical lifetime, we'll want to > early return if |mWebContents == null|. I suppose mActionMode should always be > null in that case, but we should be explicit. Done. https://codereview.chromium.org/1409443002/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3059: if (mActionMode == null) return; On 2015/10/22 00:43:56, jdduke wrote: > Can you do a sanity check that this works OK with both Chrome/WebView? > > I believe we clear the selection (thus nulling mActionMode) when we lose > explicit focus (in onFocusChanged), though we do preserve it onHide and > onWindowFocusChanged. Yeah.. I just found that it does not always work on WebView/Chrome. There are 2 possible scenarios I see: one is the translation activity displays on top of Chrome, the user can still see part of Chrome dimmed in the background. In this case, only onWindowFocusChanged. So the selection is preserved. It works in this case. The second case is when the translation activity takes fullscreen space. In this case, CVC#onHide will be called. The selection will be preserved, *but* the action mode will be dismissed, because the flag "mUnselectAllOnActionModeDismiss" only protects against selection, but not the action mode itself. Anyhow, I think in order to make it work the both cases, we can: 1. either just ignore if action mode is null in "onReceivedProcessTextResult" 2. or have a special flag to prevent the action mode from being dismissed. What do you think? https://codereview.chromium.org/1409443002/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java (right): https://codereview.chromium.org/1409443002/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java:233: // TextView in Android M. On 2015/10/22 00:45:57, jdduke wrote: > On 2015/10/22 00:43:56, jdduke wrote: > > Hmm, will calling |Replace| both replace the text and select it? Or does it > > replace it and switching to an insertion caret? If the former, we should be > OK, > > if the latter, the transition from selection to insertion will implicitly > clear > > the ActionMode. > > That is to say, either way I think we're OK here. It is the latter case: we will switch to an insertion caret. And the ActionMode is cleared. (That is actually different from TextView's behavior, which is the former case) https://codereview.chromium.org/1409443002/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/WebActionModeCallback.java:294: || Build.VERSION.SDK_INT == Build.VERSION_CODES.M; On 2015/10/22 00:45:57, jdduke wrote: > Wait, what? What about Chrome though? Sorry. My mistake here. Thanks for catching it! I moved this portion to WebView specific code in AwContents.
content/public/android lgtm with a couple more nits. https://codereview.chromium.org/1409443002/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java (right): https://codereview.chromium.org/1409443002/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:109: * If this returns {@code true} the text processing intents will be forwarded to {@link Nit: Can you replace "will be" with "should be". https://codereview.chromium.org/1409443002/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1409443002/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3065: if (mWebContents == null) return; I think we can/should safely early out here if |isSelectionEditable| is false. Should handle the two cases you mentioned, particularly the one where mActionMode is null from |onHide()|. https://codereview.chromium.org/1409443002/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3070: mWebContents.replace(result.toString()); Maybe put a TODO here and file a bug to potentially have a variant of WebContents::Replace that selects the new replacement text? I think the current behavior is fine for initially shipping this.
https://codereview.chromium.org/1409443002/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java (right): https://codereview.chromium.org/1409443002/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java:109: * If this returns {@code true} the text processing intents will be forwarded to {@link On 2015/10/22 19:52:34, jdduke wrote: > Nit: Can you replace "will be" with "should be". Done. https://codereview.chromium.org/1409443002/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1409443002/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3065: if (mWebContents == null) return; On 2015/10/22 19:52:34, jdduke wrote: > I think we can/should safely early out here if |isSelectionEditable| is false. > Should handle the two cases you mentioned, particularly the one where > mActionMode is null from |onHide()|. Done. https://codereview.chromium.org/1409443002/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3070: mWebContents.replace(result.toString()); On 2015/10/22 19:52:34, jdduke wrote: > Maybe put a TODO here and file a bug to potentially have a variant of > WebContents::Replace that selects the new replacement text? I think the current > behavior is fine for initially shipping this. Done.
The CQ bit was checked by hush@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org, jdduke@chromium.org Link to the patchset: https://codereview.chromium.org/1409443002/#ps220001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1409443002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1409443002/220001
On 2015/10/22 20:33:19, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1409443002/220001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1409443002/220001 You need an owner for content/browser/web_contents
https://codereview.chromium.org/1409443002/diff/220001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1409443002/diff/220001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3072: // TODO(hush): use a variant of replace that re-selects the replaced text. Nit: Capitalize "use"
The CQ bit was unchecked by hush@chromium.org
hush@chromium.org changed reviewers: + sievers@chromium.org
+sievers: Hi Daniel, can you take a look at content/browser/web_contents?
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/1409443002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1409443002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by hush@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org, boliu@chromium.org Link to the patchset: https://codereview.chromium.org/1409443002/#ps260001 (title: "moved comments around")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1409443002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1409443002/260001
Message was sent while issue was closed.
Committed patchset #10 (id:260001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/bc5bc8853ce851bc323a2d4fbddb25abacd0aafe Cr-Commit-Position: refs/heads/master@{#355858} |
