Chromium Code Reviews
Help | Chromium Project | Sign in
(3)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 1 week ago by Donn Denman
Modified:
2 days, 2 hours ago
Reviewers:
Theresa, aelias
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

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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+90 lines, -56 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 3 chunks +14 lines, -10 lines 0 comments Download
M content/browser/android/content_view_core_impl.h View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M content/browser/android/content_view_core_impl.cc View 1 2 3 4 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 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 2 chunks +11 lines, -0 lines 0 comments Download
M content/common/view_messages.h View 1 2 3 4 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 1 chunk +6 lines, -0 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/ContextSelectionClient.java View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/SelectionClient.java View 1 2 3 4 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 1 chunk +6 lines, -0 lines 0 comments Download
M content/renderer/render_view_impl.cc View 1 2 3 4 1 chunk +12 lines, -1 line 0 comments Download
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Messages

Total messages: 28 (13 generated)
Donn Denman
Theresa, Please take a first quick-look at this CL. I'm not sure who will need ...
2 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 ...
2 months, 1 week ago (2017-02-17 18:36:55 UTC) #8
Donn Denman
Thanks Theresa!
2 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 ...
2 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 ...
2 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months 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 ...
3 days, 2 hours ago (2017-04-27 21:31:40 UTC) #21
Donn Denman
Background details in issue 435778.
3 days, 2 hours 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 ...
3 days, 2 hours 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 ...
2 days, 3 hours 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 ...
2 days, 3 hours 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 ...
2 days, 2 hours ago (2017-04-28 21:54:42 UTC) #27
aelias
2 days, 2 hours ago (2017-04-28 22:14:33 UTC) #28
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.
Sign in to reply to this message.

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