Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2703643004:
[TTS] Add an ACK message to SelectWordAroundCaret. (Closed)
|
Created:
3 years, 10 months ago by Donn Denman Modified:
3 years, 7 months ago CC:
agrieve+watch_chromium.org, amaralp, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, donnd+watch_chromium.org, jam, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, nona+watch_chromium.org, shuchen+watch_chromium.org, James Su, Tima Vaisburd, twellington+watch_chromium.org, Theresa, yusukes+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[TTS] Add an ACK message to SelectWordAroundCaret.
Adds an ACK message so callers of SelectWordAroundCaret can know
if the selection actually happened and how much the selection was
adjusted.
BUG=692824, 435778
Review-Url: https://codereview.chromium.org/2703643004
Cr-Commit-Position: refs/heads/master@{#472596}
Committed: https://chromium.googlesource.com/chromium/src/+/701cc20866b6f07e3f3f693d0a64bd93e5219c48
Patch Set 1 #Patch Set 2 : Clean up the logic in render_view_impl.cc #
Total comments: 2
Patch Set 3 : Just fixed a typo in a comment. #
Total comments: 10
Patch Set 4 : Changed to send the message on all platforms, not just Android. #Patch Set 5 : Updated and rebased onto recently refactored Contextual Search, which shows simplifications availab… #
Total comments: 11
Patch Set 6 : Just removed an isOverlayVideoMode check. #Patch Set 7 : Added a counter to track calls to SelectWordAroundCaret without any ACK. #
Total comments: 9
Patch Set 8 : Reworked the core code to always return an ACK and check for a null frame. #
Total comments: 2
Patch Set 9 : Only changed an "auto*" to "WebLocalFrame*" in render_view_impl.cc. #Patch Set 10 : Fix a test: Update the stubs in ContextualSearchTapEventTest.java to know about the ACK. Rebase. #Patch Set 11 : Update the other test in ContextualSearchTapEventTest.java to know about the ACK. #Messages
Total messages: 73 (36 generated)
Description was changed from ========== [TTS] Add an ACK message to SelectWordAroundCaret. Adds an ACK message so callers of SelectWordAroundCaret can know if the selection actually happened and how the selection was adjusted. BUG=692824,435778 ========== to ========== [TTS] Add an ACK message to SelectWordAroundCaret. Adds an ACK message so callers of SelectWordAroundCaret can know if the selection actually happened and how much the selection was adjusted. BUG=692824,435778 ==========
The CQ bit was checked by donnd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
donnd@chromium.org changed reviewers: + twellington@chromium.org
Theresa, Please take a first quick-look at this CL. I'm not sure who will need to end up reviewing, and not sure I'm structuring everything right with the various layers. BTW I have not yet see it return false so it may not help with our invalid-tap-delay logic, but it does seem to return the right selection adjustments and I can use them for the text-tapped CL in progress.
The approach generally looks good to me. We should discuss with aelias@. https://codereview.chromium.org/2703643004/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContextualSearchClient.java (right): https://codereview.chromium.org/2703643004/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContextualSearchClient.java:33: * Acknowledges that a selectWordAroundCaret action has completed with the give result. nit: s/give/given
Thanks Theresa!
donnd@chromium.org changed reviewers: - twellington@chromium.org
https://codereview.chromium.org/2703643004/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContextualSearchClient.java (right): https://codereview.chromium.org/2703643004/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContextualSearchClient.java:33: * Acknowledges that a selectWordAroundCaret action has completed with the give result. On 2017/02/17 18:36:55, Theresa wrote: > nit: s/give/given Done.
donnd@chromium.org changed reviewers: + aelias@chromium.org
Alex, PTAL at this addition of an ACK for SelectWordAroundCaret. I'm unsure if it should be android-only, and if the structure of the piping through the layers is correct. Thanks!
https://codereview.chromium.org/2703643004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java (right): https://codereview.chromium.org/2703643004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:442: // TODO(donnd): use startAdjust and endAdjust in upcoming text-extraction CL (started). You're planning to use these offsets to issue another IPC to extract the text? Could you show me the WIP patch for that? Wouldn't it be cleaner to return the extracted text contents as part of the ack you're adding now instead of making these multiple round-trips? https://codereview.chromium.org/2703643004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:443: if (!didSelect) mHandler.handleInvalidTap(); This seems a bit upside down. How about doing nothing if the ack is false, and kicking off the UI work if the ack is true? https://codereview.chromium.org/2703643004/diff/40001/content/common/view_mes... File content/common/view_messages.h (right): https://codereview.chromium.org/2703643004/diff/40001/content/common/view_mes... content/common/view_messages.h:888: IPC_MESSAGE_ROUTED3(ViewHostMsg_SelectWordAroundCaret_ACK, Style nit: ViewHostMsg_SelectWordAroundCaretAck https://codereview.chromium.org/2703643004/diff/40001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2703643004/diff/40001/content/renderer/render... content/renderer/render_view_impl.cc:1297: #if defined(OS_ANDROID) I'd prefer it not be OS_ANDROID. This makes compile errors hard to catch and unit tests hard to write. https://codereview.chromium.org/2703643004/diff/40001/content/renderer/render... content/renderer/render_view_impl.cc:1301: bool did_select = webview()->focusedFrame()->selectWordAroundCaret(); I'm missing a bit of context; why do we set a caret in the first place? Is this only for editable boxes?
The CQ bit was checked by donnd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
aelias, PTAL. Sorry for the lack of context with this CL. Here's a summary and background. This CL is part of a change to gather surrounding text and selection offsets before showing the CS UX so we can apply machine learning to whether or not to trigger. When the user taps in plain text Blink puts a caret selection at the tap location. The current code selects the word and then gathers context to send to the server. With this change CS will gather context first, then decide whether to select the word. When the word does get selected we need to know what happened to the selection to be sure to send the right offsets to the server. I've removed the Android conditional, so this ACK message will be sent on all platforms. However we still only have a listener for the ACK implemented on Android. On other platforms the message will just be ignored, right? https://codereview.chromium.org/2703643004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java (right): https://codereview.chromium.org/2703643004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:442: // TODO(donnd): use startAdjust and endAdjust in upcoming text-extraction CL (started). On 2017/02/22 03:36:00, aelias wrote: > You're planning to use these offsets to issue another IPC to extract the text? > Could you show me the WIP patch for that? Wouldn't it be cleaner to return the > extracted text contents as part of the ack you're adding now instead of making > these multiple round-trips? Actually we plan to just use these to convert some selection offsets before sending to the server. We don't plan to issue any additional IPC (and the WIP patch isn't really ready to look at). https://codereview.chromium.org/2703643004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:443: if (!didSelect) mHandler.handleInvalidTap(); On 2017/02/22 03:36:00, aelias wrote: > This seems a bit upside down. How about doing nothing if the ack is false, and > kicking off the UI work if the ack is true? I think this will look much better once the dependent patch lands (which is a high priority to do soon). Then we'll take some action in both cases. https://codereview.chromium.org/2703643004/diff/40001/content/common/view_mes... File content/common/view_messages.h (right): https://codereview.chromium.org/2703643004/diff/40001/content/common/view_mes... content/common/view_messages.h:888: IPC_MESSAGE_ROUTED3(ViewHostMsg_SelectWordAroundCaret_ACK, On 2017/02/22 03:36:00, aelias wrote: > Style nit: ViewHostMsg_SelectWordAroundCaretAck Done. https://codereview.chromium.org/2703643004/diff/40001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2703643004/diff/40001/content/renderer/render... content/renderer/render_view_impl.cc:1297: #if defined(OS_ANDROID) On 2017/02/22 03:36:01, aelias wrote: > I'd prefer it not be OS_ANDROID. This makes compile errors hard to catch and > unit tests hard to write. Done. https://codereview.chromium.org/2703643004/diff/40001/content/renderer/render... content/renderer/render_view_impl.cc:1301: bool did_select = webview()->focusedFrame()->selectWordAroundCaret(); On 2017/02/22 03:36:00, aelias wrote: > I'm missing a bit of context; why do we set a caret in the first place? Is this > only for editable boxes? Sorry about the lack of context. When the user taps on plain text Blink creates a caret selection. When CS decides to show the UX we call selectWordAroundCaret to select the word.
FYI Alex and I talked f2f about this CL and we're going to wait until the dependent CL is ready for a full review.
aelias@, PTAL. I recently landed a major refactoring of Contextual Search, which now can make good use of this callback. This Cl shows the benefits. Feel free to IM or come by if you have questions!
Background details in issue 435778.
Looks like a nice cleanup overall. https://codereview.chromium.org/2703643004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703643004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1215: if (isOverlayVideoMode() Is this needed? The request is already suppressed in this mode, and if some complicated scenario leads us to receive an ack despite that, it seems you still want to update the InternalStateController. https://codereview.chromium.org/2703643004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1216: || !mInternalStateController.isStillWorkingOn(InternalState.START_SHOWING_TAP_UI)) { This early-return also looks a bit sketchy. Generally, when work is suppressed, there should a mechanism to queue up the work so it happens after the suppressing condition expires, but I don't see that here. Could you add such a mechanism, show me where the existing mechanism is or remove this? https://codereview.chromium.org/2703643004/diff/80001/content/common/view_mes... File content/common/view_messages.h (right): https://codereview.chromium.org/2703643004/diff/80001/content/common/view_mes... content/common/view_messages.h:873: IPC_MESSAGE_ROUTED3(ViewHostMsg_SelectWordAroundCaretAck, I'm not sure if this is planned for a later CL, but I think this and this original ViewMsg should be changed to FrameMsg/render_frame things, for OOPIF compatibility.
donnd@chromium.org changed reviewers: + twellington@chromium.org
Adding Theresa for discussion of aborted state-sequences (see these comments below). aelias@ PTAL, and let me know if it seems reasonable to do the OOPIF conversion to frame-messages in a follow-on CL. Thanks! https://codereview.chromium.org/2703643004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703643004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1215: if (isOverlayVideoMode() On 2017/04/27 22:01:27, aelias wrote: > Is this needed? The request is already suppressed in this mode, and if some > complicated scenario leads us to receive an ack despite that, it seems you still > want to update the InternalStateController. Done. https://codereview.chromium.org/2703643004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1216: || !mInternalStateController.isStillWorkingOn(InternalState.START_SHOWING_TAP_UI)) { On 2017/04/27 22:01:27, aelias wrote: > This early-return also looks a bit sketchy. Generally, when work is suppressed, > there should a mechanism to queue up the work so it happens after the > suppressing condition expires, but I don't see that here. Could you add such a > mechanism, show me where the existing mechanism is or remove this? I *think* we want to leave this isStillWorkingOn() check in place. Adding Theresa as a reviewer to double-check this logic (below). > Generally, when work is suppressed, > there should a mechanism to queue up the work so it happens after the > suppressing condition expires. That's a different model than the one we've implemented; work on the previous task should be abandoned rather than queued. Here are some likely scenarios: the user taps and we've decided to select the word, and the renderer is returning the ACK that the selection was just done. However we know that some other action occurred and that we've started a new work sequence. That could be due to the user scrolling, pausing chrome, or a second tap that followed right after the first one, etc. In all of these cases the new sequence starts with a message to the renderer to clear the selection. So, should the first-tap's word be shown in the Bar? We want the Bar to always reflect the current selection, and even though we've just gotten an ACK for that selection we also know that we're no longer processing the state-sequence associated with that first action. So abandoning the work sequence seems like the right response. Does this make sense to everyone? https://codereview.chromium.org/2703643004/diff/80001/content/common/view_mes... File content/common/view_messages.h (right): https://codereview.chromium.org/2703643004/diff/80001/content/common/view_mes... content/common/view_messages.h:873: IPC_MESSAGE_ROUTED3(ViewHostMsg_SelectWordAroundCaretAck, On 2017/04/27 22:01:27, aelias wrote: > I'm not sure if this is planned for a later CL, but I think this and this > original ViewMsg should be changed to FrameMsg/render_frame things, for OOPIF > compatibility. I think this is best done in a separate CL to keep possible issues regarding moving from view->frame separate from issues with updating CL for this ACK. Is there a bug for moving to frame for OOPIF?
https://codereview.chromium.org/2703643004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703643004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1216: || !mInternalStateController.isStillWorkingOn(InternalState.START_SHOWING_TAP_UI)) { On 2017/04/28 at 20:28:00, Donn Denman wrote: > That's a different model than the one we've implemented; work on the previous task should be abandoned rather than queued. > > Here are some likely scenarios: the user taps and we've decided to select the word, and the renderer is returning the ACK that the selection was just done. However we know that some other action occurred and that we've started a new work sequence. That could be due to the user scrolling, pausing chrome, or a second tap that followed right after the first one, etc. In all of these cases the new sequence starts with a message to the renderer to clear the selection. So, should the first-tap's word be shown in the Bar? We want the Bar to always reflect the current selection, and even though we've just gotten an ACK for that selection we also know that we're no longer processing the state-sequence associated with that first action. So abandoning the work sequence seems like the right response. > > Does this make sense to everyone? It doesn't fully make sense to me. If the ack should be abandoned, then this should happen regardless of whether the new work is "still working on" or already completed. This sounds like a race condition. https://codereview.chromium.org/2703643004/diff/80001/content/common/view_mes... File content/common/view_messages.h (right): https://codereview.chromium.org/2703643004/diff/80001/content/common/view_mes... content/common/view_messages.h:873: IPC_MESSAGE_ROUTED3(ViewHostMsg_SelectWordAroundCaretAck, On 2017/04/28 at 20:28:00, Donn Denman wrote: > On 2017/04/27 22:01:27, aelias wrote: > > I'm not sure if this is planned for a later CL, but I think this and this > > original ViewMsg should be changed to FrameMsg/render_frame things, for OOPIF > > compatibility. > > I think this is best done in a separate CL to keep possible issues regarding moving from view->frame separate from issues with updating CL for this ACK. > > Is there a bug for moving to frame for OOPIF? I'm OK with separate CL personally. The master bug is http://crbug.com/304341
aelias@ PTAL. https://codereview.chromium.org/2703643004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703643004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1216: || !mInternalStateController.isStillWorkingOn(InternalState.START_SHOWING_TAP_UI)) { On 2017/04/28 20:55:29, aelias wrote: > On 2017/04/28 at 20:28:00, Donn Denman wrote: > > That's a different model than the one we've implemented; work on the previous > task should be abandoned rather than queued. > > > > Here are some likely scenarios: the user taps and we've decided to select the > word, and the renderer is returning the ACK that the selection was just done. > However we know that some other action occurred and that we've started a new > work sequence. That could be due to the user scrolling, pausing chrome, or a > second tap that followed right after the first one, etc. In all of these cases > the new sequence starts with a message to the renderer to clear the selection. > So, should the first-tap's word be shown in the Bar? We want the Bar to always > reflect the current selection, and even though we've just gotten an ACK for that > selection we also know that we're no longer processing the state-sequence > associated with that first action. So abandoning the work sequence seems like > the right response. > > > > Does this make sense to everyone? > > It doesn't fully make sense to me. If the ack should be abandoned, then this > should happen regardless of whether the new work is "still working on" or > already completed. This sounds like a race condition. We have a simple model where we only work on the most recent input, and we just advance states to track a work-sequence on that input. If an async operation returns and we're not "still working on" the same state we know we've switched states due to a new input and we can abandon the old one. It is theoretically possible that an async reply is so slow we'd be in the same state processing the new input as we were with the old input. However, we don't think this is practically possible, so that's why we chose this simple model. Make sense? https://codereview.chromium.org/2703643004/diff/80001/content/common/view_mes... File content/common/view_messages.h (right): https://codereview.chromium.org/2703643004/diff/80001/content/common/view_mes... content/common/view_messages.h:873: IPC_MESSAGE_ROUTED3(ViewHostMsg_SelectWordAroundCaretAck, On 2017/04/28 20:55:29, aelias wrote: > On 2017/04/28 at 20:28:00, Donn Denman wrote: > > On 2017/04/27 22:01:27, aelias wrote: > > > I'm not sure if this is planned for a later CL, but I think this and this > > > original ViewMsg should be changed to FrameMsg/render_frame things, for > OOPIF > > > compatibility. > > > > I think this is best done in a separate CL to keep possible issues regarding > moving from view->frame separate from issues with updating CL for this ACK. > > > > Is there a bug for moving to frame for OOPIF? > > I'm OK with separate CL personally. The master bug is http://crbug.com/304341 Great, filed blocking bug to do that.
https://codereview.chromium.org/2703643004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703643004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1216: || !mInternalStateController.isStillWorkingOn(InternalState.START_SHOWING_TAP_UI)) { Counterpoints: - I don't agree with the claim of "not practically possible". The async reply goes through the Blink main thread. The Blink main thread can and does hang for multiple seconds at a time due to heavy Javascript. - Even if it wasn't "practically possible", it could still be happen in the artificial case of a unit test. Non-observable race conditions are a top cause of unit test flakiness. - I disagree that your model is "simpler". It may be slightly less code. But it's harder to reason about because you have a race condition.
The CQ bit was checked by donnd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Alexandre, PTAL. I just added a single counter for SelectWordAroundCaret, with the plan to move a more general solution into the InternalStateController in a separate CL after this proof-of-concept lands. How does this look (recent change in ContextualSearchManager.java only)? For the record aelias and twellingon and I had a f2f meeting to hammer out use of a counter to prevent race conditions.
https://codereview.chromium.org/2703643004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703643004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1224: || !mInternalStateController.isStillWorkingOn(InternalState.START_SHOWING_TAP_UI)) { Can this line be deleted now? It seems the scenario is fully covered by the counter.
https://codereview.chromium.org/2703643004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703643004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1224: || !mInternalStateController.isStillWorkingOn(InternalState.START_SHOWING_TAP_UI)) { On 2017/05/05 21:13:22, aelias wrote: > Can this line be deleted now? It seems the scenario is fully covered by the > counter. No, I don't think so. The scenario is a first tap starts the SelectWordAroundCaret. When the ACK calls in to here we may already have gotten a second tap and started processing it, yet not gotten to the point of calling SelectWordAroundCaret. We want to abort in that case.
https://codereview.chromium.org/2703643004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703643004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1224: || !mInternalStateController.isStillWorkingOn(InternalState.START_SHOWING_TAP_UI)) { On 2017/05/05 at 21:32:40, Donn Denman wrote: > On 2017/05/05 21:13:22, aelias wrote: > > Can this line be deleted now? It seems the scenario is fully covered by the > > counter. > > No, I don't think so. The scenario is a first tap starts the SelectWordAroundCaret. When the ACK calls in to here we may already have gotten a second tap and started processing it, yet not gotten to the point of calling SelectWordAroundCaret. We want to abort in that case. I don't get it, you increment the counter right next to setting your state machine to InternalState.START_SHOWING_TAP_UI. How could we possibly be in START_SHOWING_TAP_UI state with a nonzero counter?
https://codereview.chromium.org/2703643004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703643004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1224: || !mInternalStateController.isStillWorkingOn(InternalState.START_SHOWING_TAP_UI)) { On 2017/05/05 21:47:01, aelias wrote: > On 2017/05/05 at 21:32:40, Donn Denman wrote: > > On 2017/05/05 21:13:22, aelias wrote: > > > Can this line be deleted now? It seems the scenario is fully covered by the > > > counter. > > > > No, I don't think so. The scenario is a first tap starts the > SelectWordAroundCaret. When the ACK calls in to here we may already have gotten > a second tap and started processing it, yet not gotten to the point of calling > SelectWordAroundCaret. We want to abort in that case. > > I don't get it, you increment the counter right next to setting your state > machine to InternalState.START_SHOWING_TAP_UI. How could we possibly be in > START_SHOWING_TAP_UI state with a nonzero counter? We simply might be in some other state at the time the renderer returns with this ACK. So we need to check the current state too.
OK, never mind, I realize this is still needed for stuff like scrolling canceling the situation as well. lgtm
donnd@chromium.org changed reviewers: + nasko@chromium.org
Nasko, PTAL for security-owners at view_messages.h, or recommend a security-reviewer. We're just adding an ACK message to the existing SelectWordAroundCaret view message. Thanks!
https://codereview.chromium.org/2703643004/diff/120001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2703643004/diff/120001/content/renderer/rende... content/renderer/render_view_impl.cc:1287: blink::WebRange initial_range = webview()->FocusedFrame()->SelectionRange(); FocusedFrame() can return nullptr, it needs to be checked. https://codereview.chromium.org/2703643004/diff/120001/content/renderer/rende... content/renderer/render_view_impl.cc:1291: webview()->FocusedFrame()->SelectionRange(); This is the third call to webview()->FocusedFrame(). I don't think it can change within the scope of this method, so why not store it in a local variable?
Nasko, PTAL: Your comment got me to rewrite the whole core method because I noticed there was already an early return -- I think it should send the ACK (NACK) too. Thanks! https://codereview.chromium.org/2703643004/diff/120001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2703643004/diff/120001/content/renderer/rende... content/renderer/render_view_impl.cc:1281: if (!webview()) I noticed this early return, and think it violates the spirit of having an ACK, so I reworked this too. PTAL. https://codereview.chromium.org/2703643004/diff/120001/content/renderer/rende... content/renderer/render_view_impl.cc:1287: blink::WebRange initial_range = webview()->FocusedFrame()->SelectionRange(); On 2017/05/11 22:21:38, nasko wrote: > FocusedFrame() can return nullptr, it needs to be checked. Done. https://codereview.chromium.org/2703643004/diff/120001/content/renderer/rende... content/renderer/render_view_impl.cc:1291: webview()->FocusedFrame()->SelectionRange(); On 2017/05/11 22:21:38, nasko wrote: > This is the third call to webview()->FocusedFrame(). I don't think it can change > within the scope of this method, so why not store it in a local variable? Done.
aelias@ PTAL at render_view_impl.cc, I realized an existing early return violates the spirit of having an ACK, so I reworked the whole core portion of this change.
The CQ bit was checked by aelias@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
content/renderer lgtm https://codereview.chromium.org/2703643004/diff/140001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2703643004/diff/140001/content/renderer/rende... content/renderer/render_view_impl.cc:1289: auto* focused_frame = GetWebView()->FocusedFrame(); nit: please avoid 'auto' here, type is likely not messy-looking enough to justify.
aelias@ Thank you! twellington@ Please let me know if you're interested in taking another look (no LGTM from you yet -- but I think we're probably good). https://codereview.chromium.org/2703643004/diff/140001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2703643004/diff/140001/content/renderer/rende... content/renderer/render_view_impl.cc:1289: auto* focused_frame = GetWebView()->FocusedFrame(); On 2017/05/15 17:46:20, aelias wrote: > nit: please avoid 'auto' here, type is likely not messy-looking enough to > justify. Done.
The CQ bit was checked by donnd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please go ahead and land without my review (unless you'd like me to do another pass). I trust you aelias@
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by donnd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Committing now. Some tests needed to be updated but I don't think another round of reviews is needed. Thanks everyone!
The CQ bit was checked by donnd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org Link to the patchset: https://codereview.chromium.org/2703643004/#ps200001 (title: "Update the other test in ContextualSearchTapEventTest.java to know about the ACK.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
donnd@chromium.org changed reviewers: + creis@chromium.org, tedchoc@chromium.org - nasko@chromium.org
Charlie, PTAL at view_messages.h. For OOPIF I plan to switch the SelectWordAroundCaret from a View-message to a Frame-message in a later CL (crbug.com/716618 is tracking that). Ted, PTAL at content_view_core_impl.cc/h Thanks!
creis@chromium.org changed reviewers: + kenrb@chromium.org
On 2017/05/17 19:04:48, Donn Denman wrote: > Charlie, PTAL at view_messages.h. For OOPIF I plan to switch the > SelectWordAroundCaret from a View-message to a Frame-message in a later CL > (crbug.com/716618 is tracking that). Thanks for pointing me to 716618-- glad you all caught it! That said, I'm not an IPC reviewer. kenrb@, can you review it?
On 2017/05/17 19:11:28, Charlie Reis wrote: > On 2017/05/17 19:04:48, Donn Denman wrote: > > Charlie, PTAL at view_messages.h. For OOPIF I plan to switch the > > SelectWordAroundCaret from a View-message to a Frame-message in a later CL > > (crbug.com/716618 is tracking that). > > Thanks for pointing me to 716618-- glad you all caught it! > > That said, I'm not an IPC reviewer. kenrb@, can you review it? content / android lgtm
ipc lgtm
The CQ bit was checked by donnd@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1495055366323540,
"parent_rev": "233e1b3593cd4a9565e2ffef1a9ec97207d740e2", "commit_rev":
"701cc20866b6f07e3f3f693d0a64bd93e5219c48"}
Message was sent while issue was closed.
Description was changed from ========== [TTS] Add an ACK message to SelectWordAroundCaret. Adds an ACK message so callers of SelectWordAroundCaret can know if the selection actually happened and how much the selection was adjusted. BUG=692824,435778 ========== to ========== [TTS] Add an ACK message to SelectWordAroundCaret. Adds an ACK message so callers of SelectWordAroundCaret can know if the selection actually happened and how much the selection was adjusted. BUG=692824,435778 Review-Url: https://codereview.chromium.org/2703643004 Cr-Commit-Position: refs/heads/master@{#472596} Committed: https://chromium.googlesource.com/chromium/src/+/701cc20866b6f07e3f3f693d0a64... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/701cc20866b6f07e3f3f693d0a64... |
