Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(9)

Issue 2703643004: [TTS] Add an ACK message to SelectWordAroundCaret. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 1 week ago by Donn Denman
Modified:
1 month, 1 week 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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+141 lines, -71 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchContext.java View 1 2 3 4 3 chunks +4 lines, -45 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java View 1 2 3 4 5 6 7 8 9 6 chunks +25 lines, -11 lines 0 comments Download
M chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchTapEventTest.java View 1 2 3 4 5 6 7 8 9 10 5 chunks +31 lines, -10 lines 0 comments Download
M content/browser/android/content_view_core_impl.h View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -0 lines 0 comments Download
M content/browser/android/content_view_core_impl.cc View 1 2 3 4 5 6 7 8 1 chunk +11 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_android.h View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_android.cc View 1 2 3 4 5 6 7 8 9 2 chunks +11 lines, -0 lines 0 comments Download
M content/common/view_messages.h View 1 2 3 4 5 6 7 1 chunk +7 lines, -0 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java View 1 2 3 4 5 6 7 8 9 1 chunk +6 lines, -0 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/SelectionClient.java View 1 2 3 4 5 6 7 1 chunk +10 lines, -0 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java View 1 2 3 4 5 6 7 8 9 1 chunk +6 lines, -0 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/SmartSelectionClient.java View 1 2 3 4 5 6 7 1 chunk +3 lines, -0 lines 0 comments Download
M content/renderer/render_view_impl.cc View 1 2 3 4 5 6 7 8 9 1 chunk +21 lines, -5 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 73 (36 generated)
Donn Denman
Theresa, Please take a first quick-look at this CL. I'm not sure who will need ...
4 months, 1 week ago (2017-02-17 16:13:46 UTC) #7
Theresa
The approach generally looks good to me. We should discuss with aelias@. https://codereview.chromium.org/2703643004/diff/20001/content/public/android/java/src/org/chromium/content/browser/ContextualSearchClient.java File content/public/android/java/src/org/chromium/content/browser/ContextualSearchClient.java ...
4 months, 1 week ago (2017-02-17 18:36:55 UTC) #8
Donn Denman
Thanks Theresa!
4 months, 1 week ago (2017-02-17 19:08:17 UTC) #9
Donn Denman
https://codereview.chromium.org/2703643004/diff/20001/content/public/android/java/src/org/chromium/content/browser/ContextualSearchClient.java File content/public/android/java/src/org/chromium/content/browser/ContextualSearchClient.java (right): https://codereview.chromium.org/2703643004/diff/20001/content/public/android/java/src/org/chromium/content/browser/ContextualSearchClient.java#newcode33 content/public/android/java/src/org/chromium/content/browser/ContextualSearchClient.java:33: * Acknowledges that a selectWordAroundCaret action has completed with ...
4 months, 1 week ago (2017-02-17 19:09:14 UTC) #11
Donn Denman
Alex, PTAL at this addition of an ACK for SelectWordAroundCaret. I'm unsure if it should ...
4 months, 1 week ago (2017-02-17 19:11:02 UTC) #13
aelias
https://codereview.chromium.org/2703643004/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java (right): https://codereview.chromium.org/2703643004/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java#newcode442 chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:442: // TODO(donnd): use startAdjust and endAdjust in upcoming text-extraction ...
4 months ago (2017-02-22 03:36:01 UTC) #14
Donn Denman
aelias, PTAL. Sorry for the lack of context with this CL. Here's a summary and ...
4 months ago (2017-02-22 23:13:02 UTC) #19
Donn Denman
FYI Alex and I talked f2f about this CL and we're going to wait until ...
3 months, 4 weeks ago (2017-02-27 18:53:42 UTC) #20
Donn Denman
aelias@, PTAL. I recently landed a major refactoring of Contextual Search, which now can make ...
1 month, 4 weeks ago (2017-04-27 21:31:40 UTC) #21
Donn Denman
Background details in issue 435778.
1 month, 4 weeks ago (2017-04-27 21:36:57 UTC) #22
aelias
Looks like a nice cleanup overall. https://codereview.chromium.org/2703643004/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703643004/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java#newcode1215 chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1215: if (isOverlayVideoMode() Is ...
1 month, 4 weeks ago (2017-04-27 22:01:27 UTC) #23
Donn Denman
Adding Theresa for discussion of aborted state-sequences (see these comments below). aelias@ PTAL, and let ...
1 month, 4 weeks ago (2017-04-28 20:28:00 UTC) #25
aelias
https://codereview.chromium.org/2703643004/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703643004/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java#newcode1216 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 ...
1 month, 4 weeks ago (2017-04-28 20:55:30 UTC) #26
Donn Denman
aelias@ PTAL. https://codereview.chromium.org/2703643004/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703643004/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java#newcode1216 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 ...
1 month, 4 weeks ago (2017-04-28 21:54:42 UTC) #27
aelias
https://codereview.chromium.org/2703643004/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703643004/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java#newcode1216 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 ...
1 month, 4 weeks ago (2017-04-28 22:14:33 UTC) #28
Donn Denman
Alexandre, PTAL. I just added a single counter for SelectWordAroundCaret, with the plan to move ...
1 month, 3 weeks ago (2017-05-05 20:48:57 UTC) #33
aelias
https://codereview.chromium.org/2703643004/diff/120001/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703643004/diff/120001/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java#newcode1224 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? ...
1 month, 3 weeks ago (2017-05-05 21:13:22 UTC) #34
Donn Denman
https://codereview.chromium.org/2703643004/diff/120001/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703643004/diff/120001/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java#newcode1224 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: > ...
1 month, 3 weeks ago (2017-05-05 21:32:40 UTC) #35
aelias
https://codereview.chromium.org/2703643004/diff/120001/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703643004/diff/120001/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java#newcode1224 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 ...
1 month, 3 weeks ago (2017-05-05 21:47:01 UTC) #36
Donn Denman
https://codereview.chromium.org/2703643004/diff/120001/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703643004/diff/120001/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java#newcode1224 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: > ...
1 month, 3 weeks ago (2017-05-05 22:00:36 UTC) #37
aelias
OK, never mind, I realize this is still needed for stuff like scrolling canceling the ...
1 month, 3 weeks ago (2017-05-05 23:36:05 UTC) #38
Donn Denman
Nasko, PTAL for security-owners at view_messages.h, or recommend a security-reviewer. We're just adding an ACK ...
1 month, 2 weeks ago (2017-05-11 18:35:21 UTC) #40
nasko (slow)
https://codereview.chromium.org/2703643004/diff/120001/content/renderer/render_view_impl.cc File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2703643004/diff/120001/content/renderer/render_view_impl.cc#newcode1287 content/renderer/render_view_impl.cc:1287: blink::WebRange initial_range = webview()->FocusedFrame()->SelectionRange(); FocusedFrame() can return nullptr, it ...
1 month, 2 weeks ago (2017-05-11 22:21:38 UTC) #41
Donn Denman
Nasko, PTAL: Your comment got me to rewrite the whole core method because I noticed ...
1 month, 2 weeks ago (2017-05-11 23:05:47 UTC) #42
Donn Denman
aelias@ PTAL at render_view_impl.cc, I realized an existing early return violates the spirit of having ...
1 month, 1 week ago (2017-05-15 17:36:05 UTC) #43
aelias
content/renderer lgtm https://codereview.chromium.org/2703643004/diff/140001/content/renderer/render_view_impl.cc File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2703643004/diff/140001/content/renderer/render_view_impl.cc#newcode1289 content/renderer/render_view_impl.cc:1289: auto* focused_frame = GetWebView()->FocusedFrame(); nit: please avoid ...
1 month, 1 week ago (2017-05-15 17:46:21 UTC) #46
Donn Denman
aelias@ Thank you! twellington@ Please let me know if you're interested in taking another look ...
1 month, 1 week ago (2017-05-15 21:13:14 UTC) #47
Theresa
Please go ahead and land without my review (unless you'd like me to do another ...
1 month, 1 week ago (2017-05-15 23:44:21 UTC) #50
Donn Denman
Committing now. Some tests needed to be updated but I don't think another round of ...
1 month, 1 week ago (2017-05-17 18:35:18 UTC) #57
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2703643004/200001
1 month, 1 week ago (2017-05-17 18:36:49 UTC) #60
commit-bot: I haz the power
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_presubmit/builds/440074)
1 month, 1 week ago (2017-05-17 18:59:00 UTC) #62
Donn Denman
Charlie, PTAL at view_messages.h. For OOPIF I plan to switch the SelectWordAroundCaret from a View-message ...
1 month, 1 week ago (2017-05-17 19:04:48 UTC) #64
Charlie Reis (slow)
On 2017/05/17 19:04:48, Donn Denman wrote: > Charlie, PTAL at view_messages.h. For OOPIF I plan ...
1 month, 1 week ago (2017-05-17 19:11:28 UTC) #66
Ted C
On 2017/05/17 19:11:28, Charlie Reis wrote: > On 2017/05/17 19:04:48, Donn Denman wrote: > > ...
1 month, 1 week ago (2017-05-17 20:49:40 UTC) #67
kenrb
ipc lgtm
1 month, 1 week ago (2017-05-17 21:08:59 UTC) #68
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2703643004/200001
1 month, 1 week ago (2017-05-17 21:10:35 UTC) #70
commit-bot: I haz the power
1 month, 1 week ago (2017-05-17 23:11:47 UTC) #73
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/701cc20866b6f07e3f3f693d0a64...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cb946e318