Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2703473002:
[TTS] Extract tapped text before showing UI. (Closed)
|
Created:
3 years, 10 months ago by Donn Denman Modified:
3 years, 7 months ago CC:
chromium-reviews, twellington+watch_chromium.org, donnd+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[TTS] Extract tapped text before showing UI.
Reworks Tap gesture recognition and processing to use a new Internal
State Controller. This controller drives the ContextualSearchManager
through its sequence of state transitions. Now the manager is just
a library of operations, and the overall flow of control is handled
exclusively by the new state controller.
Now text is extracted before showing any UI. This is needed so we
can apply ML to decide when to show the UI.
The Java Context now tracks a small section of content of the Context
around the Selection. Context now has a hack to figure out how the
content changed when a word is selected with SelectWordAroundCaret.
This should be fixed soon by adding an ACK to SelectWordAroundCaret.
Stopped using the GestureStateListener#onSingleTap override so we
should be able to remove that method from the interface in a future
CL.
The SelectionController no longer needs to have a delay for a possible
invalid tap because there's a state that waits for a possible tap
nearby a previous tap.
BUG=692824
Review-Url: https://codereview.chromium.org/2703473002
Cr-Commit-Position: refs/heads/master@{#467522}
Committed: https://chromium.googlesource.com/chromium/src/+/e78147e28cc531df52ba73e0efe49d0401d28f6b
Patch Set 1 #Patch Set 2 : Added a Java context. Builds but has issues with disposing of the context. #Patch Set 3 : Works by matching the selection in the icing text. Does not expand the selection correctly. #Patch Set 4 : Added State management. #Patch Set 5 : Minor update. #
Total comments: 16
Patch Set 6 : Renamed classes and addressed Theresa's initial quick-review. #Patch Set 7 : Enhanced dependency injection for CSM, for archiving. #Patch Set 8 : Removed the refactoring for dependency injection of CSM for better testing (see CL 2786313005 for a… #Patch Set 9 : Merged invalid-tap-delay into the state controller. #
Total comments: 13
Patch Set 10 : Addressed Theresa's comments by making an anonymous instance implement the state controller interfa… #Patch Set 11 : Fixed invalid-tap handling, made more robust, updated initially failing tests. #Patch Set 12 : Minor fixes to Retap, empty-surrounding-text issue. #Patch Set 13 : Reworked icing notification, lots of cleanup, and a rebase. #
Total comments: 59
Patch Set 14 : Updated comments in response to Theresa's review today. #Patch Set 15 : Added some tests, fixed some tests, added DCHECKS to native context, rebased. #
Total comments: 10
Patch Set 16 : Moved handleHideContextualSearch into hideContextualSearchUI, and updated comments in response to T… #Depends on Patchset: Messages
Total messages: 40 (21 generated)
donnd@chromium.org changed reviewers: + twellington@chromium.org
Theresa, please take a quick look sometime soon. This isn't ready for a real review yet, but it's worth looking at the overall structure and naming. I'm interested in your feelings about this approach. I had a better description of the CL in the log, but can't seem to access it remotely and chromiumcodereview doesn't update that, which I find unhelpful. Here's what I remember saying: This just started working with the basic functionality for Tap and Long-press but is still very much a WIP. I have not tested interrupting the flow, verified that control actions are not being taken by the Manager, etc. I noticed nothing seems to prefetch. I'd love to get rid of the invalidTapNotification in the ContextualSearchSelectionController. I'd start by reading ContextualSearchStateController. The ContextualSearchStateTest works, and running it is nice and fast. :-) Thanks in advance!
https://codereview.chromium.org/2703473002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:72: ContextualSearchStateControlled, SelectionClient { There are a lot of interfaces listed here. I think prior to this CL it was confusing how ContextualSearchSelectionHandler and SelectionClient intersected. With the new interface, I think there is additional overlap between ContextualSearchStateControlled and ContextualSearchManagementDelegate. For example: ContextualSearchManagementDelegate#dismissContextualSearchBar(): when should this be called vs ContextualSearchStateControlled#hideContextualSearchUx()? ContextualSearchManagementDelegate#logCurrentState(): now that there's a state machine, it seems odd that the management delegate would expose a method logging state. In general, the CSStateControlled object is responsible for handling state-controlled asynchronous tasks and the CSManagementDelegate is responsible for global management functionality. The CSStateController manages global state, so the division of responsibilities between the two is murky. https://codereview.chromium.org/2703473002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchStateControlled.java (right): https://codereview.chromium.org/2703473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchStateControlled.java:18: void hideContextualSearchUx(StateChangeReason reason); nit: UX = user experience, UI = user interface, so this should probably be hideContextualSearchUI() https://codereview.chromium.org/2703473002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchStateController.java (right): https://codereview.chromium.org/2703473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchStateController.java:39: private final ContextualSearchStateControlled mControlled; I'd prefer a different name for this class. Maybe ContextualSearchStateDelegate or ContextualSearchStateHandler, since this object is responsible for taking action when state changes. https://codereview.chromium.org/2703473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchStateController.java:46: IDLE, Is this equivalent to the panel "closed" state (as opposed to peeking/expanded/maximized)? https://codereview.chromium.org/2703473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchStateController.java:48: // This is a sequence of transition states needed to get to the SHOWING_TAP resting state. s/SHOWING_STATE/SHOWING_LONGPRESS_SEARCH? https://codereview.chromium.org/2703473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchStateController.java:51: EXTENDING_SELECTION, // needed??? Would this be used when the user is dragging the selection pins? https://codereview.chromium.org/2703473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchStateController.java:59: DECIDING_SUPPRESSION, I expected tap suppression be synchronous (for now)? https://codereview.chromium.org/2703473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchStateController.java:146: void notifyFinishedWorkOn(State state) { There's nothing here enforcing the order of state transitions -- for example, I could call notifyFinishedWorkOn(SELECTING_WORD) even if the previous state were GATHERING_SURROUNDINGS. Do we want to throw errors if we get an unexpected notifyFinishedWorkOn relative to the current mState?
Renamed several classes based on your great feedback: State --> InternalState CSStateController --> CSInternalStateController CSStateControlled --> CSInternalStateHandler (where CS is just an abbreviation) I'll ping you when this is ready for another review! https://codereview.chromium.org/2703473002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:72: ContextualSearchStateControlled, SelectionClient { On 2017/03/28 15:52:55, Theresa wrote: > There are a lot of interfaces listed here. I think prior to this CL it was > confusing how ContextualSearchSelectionHandler and SelectionClient intersected. > With the new interface, I think there is additional overlap between > ContextualSearchStateControlled and > ContextualSearchManagementDelegate. > > > For example: > > ContextualSearchManagementDelegate#dismissContextualSearchBar(): when should > this be called vs ContextualSearchStateControlled#hideContextualSearchUx()? > > > ContextualSearchManagementDelegate#logCurrentState(): now that there's a state > machine, it seems odd that the management delegate would expose a method logging > state. > > > In general, the CSStateControlled object is responsible for handling > state-controlled asynchronous tasks and the CSManagementDelegate is responsible > for global management functionality. The CSStateController manages global state, > so the division of responsibilities between the two is murky. Hopefully this is clear now that the State is an InternalState of the CS Manager. The CSManagementDelegate is an interface that other parts of Chrome can use to control the CSManager. The CSInternalStateHandler takes care of internal state transitions when handling gestures. There still may be confusion, but I think this renaming helps a lot. Thanks for pointing out these issues! https://codereview.chromium.org/2703473002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchStateControlled.java (right): https://codereview.chromium.org/2703473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchStateControlled.java:18: void hideContextualSearchUx(StateChangeReason reason); On 2017/03/28 15:52:55, Theresa wrote: > nit: UX = user experience, UI = user interface, so this should probably be > hideContextualSearchUI() Done here and below. https://codereview.chromium.org/2703473002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchStateController.java (right): https://codereview.chromium.org/2703473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchStateController.java:39: private final ContextualSearchStateControlled mControlled; On 2017/03/28 15:52:55, Theresa wrote: > I'd prefer a different name for this class. Maybe ContextualSearchStateDelegate > or ContextualSearchStateHandler, since this object is responsible for taking > action when state changes. Changed to ContextualSearchInternalStateHandler. https://codereview.chromium.org/2703473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchStateController.java:46: IDLE, On 2017/03/28 15:52:55, Theresa wrote: > Is this equivalent to the panel "closed" state (as opposed to > peeking/expanded/maximized)? Yes, updated the comment to reflect that the IDLE state displays no UI. https://codereview.chromium.org/2703473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchStateController.java:48: // This is a sequence of transition states needed to get to the SHOWING_TAP resting state. On 2017/03/28 15:52:55, Theresa wrote: > s/SHOWING_STATE/SHOWING_LONGPRESS_SEARCH? Done. https://codereview.chromium.org/2703473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchStateController.java:51: EXTENDING_SELECTION, // needed??? On 2017/03/28 15:52:55, Theresa wrote: > Would this be used when the user is dragging the selection pins? Yep. Still not sure it's needed, but we're supposed to send text to Icing so we might need to gather surrounding text again, etc. https://codereview.chromium.org/2703473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchStateController.java:59: DECIDING_SUPPRESSION, On 2017/03/28 15:52:55, Theresa wrote: > I expected tap suppression be synchronous (for now)? This CL adds both sequencing and handling of async operations, and I think the sequencing part is still valuable for operations like this that are currently synchronous but hard to follow and insert into the workflow. Updated the documentation to reflect that operations don't need to be async. https://codereview.chromium.org/2703473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchStateController.java:146: void notifyFinishedWorkOn(State state) { On 2017/03/28 15:52:55, Theresa wrote: > There's nothing here enforcing the order of state transitions -- for example, I > could call notifyFinishedWorkOn(SELECTING_WORD) even if the previous state were > GATHERING_SURROUNDINGS. > > Do we want to throw errors if we get an unexpected notifyFinishedWorkOn relative > to the current mState? I tried to do this with the initial implementation but there's a problem that arises when operations are in progress when a new gesture comes in. For example we could be in the middle of a long Resolve when a gesture starts and we process several steps in that sequence. Now the Resolve response comes in and we need to realize we're no longer in the same sequence. I couldn't think of a simple way to handle both cases, and I think the graceful-abort case is more important. I still hope to address enforcing this in tests, but don't yet have that figured out.
Description was changed from
==========
Tap extract simple. Compiles and runs, tests out of date.
BUG=692824
==========
to
==========
[TTS] Extract tapped text before showing UI.
Reworks Tap gesture recognition and processing to use a new
Internal State Controller. This controller drives the
ContextualSearchManager through its sequence of state
transitions needed to handle a user gesture.
Now text is extracted before showing any UI so the text can
be evaluated by an ML system for UI suppression.
This is still a WIP! Builds and runs and has a new basic test.
Still lots of work to do to update tests, make sure the logic
and sequencing is correct, interrupting a sequence works, etc.
Basic functionality of Tap and Longpress seems OK except for
prefetch.
BUG=692824
==========
Description was changed from
==========
[TTS] Extract tapped text before showing UI.
Reworks Tap gesture recognition and processing to use a new
Internal State Controller. This controller drives the
ContextualSearchManager through its sequence of state
transitions needed to handle a user gesture.
Now text is extracted before showing any UI so the text can
be evaluated by an ML system for UI suppression.
This is still a WIP! Builds and runs and has a new basic test.
Still lots of work to do to update tests, make sure the logic
and sequencing is correct, interrupting a sequence works, etc.
Basic functionality of Tap and Longpress seems OK except for
prefetch.
BUG=692824
==========
to
==========
[TTS] Extract tapped text before showing UI.
Reworks Tap gesture recognition and processing to use a new
Internal State Controller. This controller drives the
ContextualSearchManager through its sequence of state
transitions needed to handle a user gesture.
Now text is extracted before showing any UI so the text can
be evaluated by an ML system for UI suppression.
This is still a WIP! Builds and runs and has a new basic test.
Still lots of work to do to update tests, make sure the logic
and sequencing is correct, interrupting a sequence works, etc.
Basic functionality of Tap and Longpress seems OK except for
retap.
BUG=692824
==========
Theresa, this is ready for quick-look #2 sometime next week. Thanks!
Theresa, the most recent patch that I uploaded might not be very functional, so don't bother patching and trying. I was still trying to get the re-tap logic to work, and I think basic Tap functionality was still broken when it was uploaded. But it was working pretty well before tackling retap! :-) Just a second look at overall structure and naming is probably best for now. Also if you find you're busy all week and don't get a chance to review at all, that's fine too (I'll just update and resend it when I return from vacation). :-) On Mar 31, 2017 4:16 PM, <donnd@chromium.org> wrote: Theresa, this is ready for quick-look #2 sometime next week. Thanks! https://codereview.chromium.org/2703473002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2703473002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateController.java (right): https://codereview.chromium.org/2703473002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateController.java:117: // does not yet have an ACK so we infer that it's complete when the selection change -- or How is the ACK progressing? Is it blocked on something? https://codereview.chromium.org/2703473002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703473002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:338: public void hideContextualSearch(StateChangeReason reason) { I think that hideContextualSearch() vs hideContextualSearchUi() vs dismissContextualSearchBar() vs onCloseContextualSearch() is confusing. Ideally we would only expose one public method that gets rid of the Contextual Search UI. https://codereview.chromium.org/2703473002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:409: * Shows the Contextual Search UX. Calls back into onGetContextualSearchQueryResponse. I don't think onGetContextualSearchQueryResponse() exists anymore https://codereview.chromium.org/2703473002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:413: private void showContextualSearch(StateChangeReason stateChangeReason) { Can this just be the showContextualSearchUi() override for ContextualSEarchInternalStateHandler? Otherwise, let's rename to showContextualSearchInternal to help differentiate between the two methods. https://codereview.chromium.org/2703473002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:414: mInternalStateController.notifyStartingWorkOn(mInternalStateController.getState()); Should we use CSInternalStateController#SHOW_FULL_TAP_UI directly instead of getState()? https://codereview.chromium.org/2703473002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1522: mNetworkCommunicator.startSearchTermResolutionRequest(selection); When looking at ContextualSearchManager, I think it now takes extra work to sort the different methods. Now we have: /** Starts a Resolve request to our server for the best Search Term. */ public void resolveSearchTerm() /** Starts a Search Term Resolution request. **/ public void startSearchTermResolutionRequest(String selection);
Description was changed from
==========
[TTS] Extract tapped text before showing UI.
Reworks Tap gesture recognition and processing to use a new
Internal State Controller. This controller drives the
ContextualSearchManager through its sequence of state
transitions needed to handle a user gesture.
Now text is extracted before showing any UI so the text can
be evaluated by an ML system for UI suppression.
This is still a WIP! Builds and runs and has a new basic test.
Still lots of work to do to update tests, make sure the logic
and sequencing is correct, interrupting a sequence works, etc.
Basic functionality of Tap and Longpress seems OK except for
retap.
BUG=692824
==========
to
==========
[TTS] Extract tapped text before showing UI.
Reworks Tap gesture recognition and processing to use a new
State Controller. This controller drives the ContextualSearchManager
through its sequence of state transitions.
Now text is extracted before showing any UI.
Still lots of work to do to update tests, make sure the logic
and sequencing is correct, handle invalid taps, etc.
Basic functionality of Tap and Long-press is done.
BUG=692824
==========
Theresa, your comments about new public methods in the manager raised an important issue and helped me figure out how to prevent adding new ones: Now the manager creates an anonymous instance that implements the public methods, so it doesn't have to implement the new API itself. In future CLs we should be able to do this (or use inner classes) for some of the other interfaces the manager implements too. I'll let you know when I think it's ready for another look. Thanks for the thoughtful review! https://codereview.chromium.org/2703473002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateController.java (right): https://codereview.chromium.org/2703473002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateController.java:117: // does not yet have an ACK so we infer that it's complete when the selection change -- or On 2017/04/05 20:58:30, Theresa wrote: > How is the ACK progressing? Is it blocked on something? There's a separate CL that sends that ACK message, which I plan to update after this lands. (CL https://chromiumcodereview.appspot.com/2703643004/) aelias@ wanted to see the context for the ACK and I think that's best done by showing how the selection-matching in ContextualSearchContext#updateContextFromSelection can be removed with the ACK. https://codereview.chromium.org/2703473002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703473002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:70: ContextualSearchInternalStateHandler, SelectionClient { I removed the ContextualSearchInternalStateHandler from this list, which solves the proliferation of public entry points issue. Also added a TODO for additional refactoring along these lines. https://codereview.chromium.org/2703473002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:338: public void hideContextualSearch(StateChangeReason reason) { On 2017/04/05 20:58:30, Theresa wrote: > I think that hideContextualSearch() vs hideContextualSearchUi() vs > dismissContextualSearchBar() vs onCloseContextualSearch() is confusing. Ideally > we would only expose one public method that gets rid of the Contextual Search > UI. Thanks for pointing out all these confusing ways to hide! HideContextualSearchUi is now scoped to an anonymous instance. Hopefully we can create similar implementers for some of the others rather than exposing them at the manager level. https://codereview.chromium.org/2703473002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:409: * Shows the Contextual Search UX. Calls back into onGetContextualSearchQueryResponse. On 2017/04/05 20:58:30, Theresa wrote: > I don't think onGetContextualSearchQueryResponse() exists anymore Done. https://codereview.chromium.org/2703473002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:413: private void showContextualSearch(StateChangeReason stateChangeReason) { On 2017/04/05 20:58:30, Theresa wrote: > Can this just be the showContextualSearchUi() override for > ContextualSEarchInternalStateHandler? > > Otherwise, let's rename to showContextualSearchInternal to help differentiate > between the two methods. I split showContextualSearchUi into two methods (for Tap and Longpress) so now there's less chance of confusion. Let me know if you'd still like the name changed. https://codereview.chromium.org/2703473002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:414: mInternalStateController.notifyStartingWorkOn(mInternalStateController.getState()); On 2017/04/05 20:58:30, Theresa wrote: > Should we use CSInternalStateController#SHOW_FULL_TAP_UI directly instead of > getState()? Done: Now there are two callers that can do the notification with literal values (for Tap vs Long-press). https://codereview.chromium.org/2703473002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1522: mNetworkCommunicator.startSearchTermResolutionRequest(selection); On 2017/04/05 20:58:30, Theresa wrote: > When looking at ContextualSearchManager, I think it now takes extra work to sort > the different methods. > > Now we have: > > /** Starts a Resolve request to our server for the best Search Term. */ > public void resolveSearchTerm() > > /** Starts a Search Term Resolution request. **/ > public void startSearchTermResolutionRequest(String selection); Fixed by making this new resolveSearchTerm scoped by an anonymous instance.
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Theresa, this is ready for an in-depth review with a few caveats: 1) The ContextualSearchTapEventTest.java is all screwed up. I'll be working on fixing some outstanding problems there and adding a test that ensures a full sequence of start/finish states on Tap. 2) There's a new bug that the bar hides when you do a second Tap (tap on a Tap selection to show the pins). 3) There's a new bug that I don't quite understand where taping really fast many times might show "..." or a whitespace gap when showing the resolving info in the Bar. 4) Ignore the new test at the very end of ContextualSearchManagerTest.java. Also there's a new unit test that seems to work well (I love the speed), and I think I'll do a CL to put an OWNERS file in that directory and update all our other OWNERS to have TEAM info. Let me know if you have questions!
Description was changed from
==========
[TTS] Extract tapped text before showing UI.
Reworks Tap gesture recognition and processing to use a new
State Controller. This controller drives the ContextualSearchManager
through its sequence of state transitions.
Now text is extracted before showing any UI.
Still lots of work to do to update tests, make sure the logic
and sequencing is correct, handle invalid taps, etc.
Basic functionality of Tap and Long-press is done.
BUG=692824
==========
to
==========
[TTS] Extract tapped text before showing UI.
Reworks Tap gesture recognition and processing to use a new Internal
State Controller. This controller drives the ContextualSearchManager
through its sequence of state transitions. Now the manager is just
a library of operations, and the overall flow of control is handled
exclusively by the new state controller.
Now text is extracted before showing any UI. This is needed so we
can apply ML to decide when to show the UI.
The Java Context now tracks a small section of content of the Context
around the Selection. Context now has a hack to figure out how the
content changed when a word is selected with SelectWordAroundCaret.
This should be fixed soon by adding an ACK to SelectWordAroundCaret.
Stopped using the GestureStateListener#onSingleTap override so we
should be able to remove that method from the interface in a future
CL.
The SelectionController no longer needs to have a delay for a possible
invalid tap because there's a state that waits for a possible tap
nearby a previous tap.
BUG=692824
==========
https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java (right): https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:554: requestUpdate(); Is this fixing a pre-existing bug? https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchContext.java (right): https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchContext.java:165: * TODO(donnd): This method of finding the adjustment to the selection is unreliable! How unreliable? Is this something we need to fix before this patch makes it into Stable? Dev? https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchContext.java:176: && !TextUtils.isEmpty(mSurroundingText) && !TextUtils.isEmpty(selection)) { Let's do an early return instead to avoid one level of nesting/indentation. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchContext.java:194: * @param startAdjust Finish JavaDoc https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateController.java (right): https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateController.java:45: * States can be "start states" which can be passed to #enter, or "transitional states" which nit s/#enter/#enter() https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateController.java:52: // destroyed. nit: Can these be JavaDoc so that IDEs show in-line help on hover? https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateController.java:77: // Start showing the Tap UI. Currently this means select the word tapped. nit: select the word tapped - > select the word that was tapped? https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateController.java:127: * @param reason the reason for the reset nit: The Chromium code base capitalize the start of parameter descriptions in JavaDocs. We've discussed this before, right? Where did we land? https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateController.java:288: reset(StateChangeReason.BASE_PAGE_TAP); If we get another tap near by, we'll start another action (TAP_RECOGNIZED?) rather than calling finishWorkingOn? https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:89: // This setting is not critical: in practice it determines how long to wait after an invalid s/invalid for/invalid tap for https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:91: private static final int TAP_NEAR_PREVIOUS_DETECTION_DELAY_MS = 100; We should revisit this delay after we gather surroundings and decide suppression before selection is established since that may take longer than 100ms. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:498: hideContextualSearch(StateChangeReason.UNKNOWN); Should we fall back to a verbatim search rather than hiding the UI? https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:644: if (!mInternalStateController.isStillWorkingOn(InternalState.RESOLVING)) return; On a really slow network, is it possible for something like this to happen? 1) Send resolution request 2) User taps nearby to start another search, another resolution request is sent 3) First resolution request comes in, the internal state is resolving so we handle the resolution response even though it's for an old request https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1224: // TODO(donnd): Add support for this call from selectWordAroundCaret and add @Override Is this method being called yet? If not, should we pull it out for now? https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1225: public void selectWordAroundCaretAck(boolean didSelect, int startAdjust, int endAdjust) { Add a JavaDoc for this method https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1280: if (mIsAccessibilityModeEnabled) return; If we return early because we're in accessibility mode, will the state handler be left in InternalState.DECIDING_SUPPRESSION? https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1314: if (mIsAccessibilityModeEnabled) return; Same question about early return and leaving the controller in an invalid state here. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1333: mInternalStateController.enter(InternalState.LONG_PRESS_RECOGNIZED); It may be worth documenting here what happens when we enter long_press_recognized (and why we only "recognize" a long press but start showing the ui for tap). I think this is what's happening, is my understanding correct? LONG_PRESS_RECOGNIZED -> transitionTo(InternalState.GATHERING_SURROUNDINGS) though mechanisms in the internal state handler -> CSInternalStateHandler#gatherSurroundingText() -> finish gathering, notify state handler, transitionTo(InternalState.SHOWING_LONGPRESS_SEARCH) https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1418: boolean isTap = mSelectionController.getSelectionType() == SelectionType.TAP; nit: add a blank line before this one https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1440: if (baseWebContents != null && mPolicy.isTapSupported()) { In a follow up CL, let's revisit when we drop out if tap isn't supported. Ideally we would stop processing as soon as possible to avoid gathering surroundings, deciding suppression, etc. if we're going to end up dropping everything because tap isn't supported. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1487: public void hideContextualSearchUi(StateChangeReason reason) { Moving this to a private internal implementation certainly helps a lot with the issue of knowing whether to call hideContextualSearch() or hideContextualSearchUi()! It is interesting, though, that rather than one method to hide the UI we now have: CSManager#hideContextualSearch() -> ContextualSearchInternalStateController#reset() -> CSInternalStateHandler#hideContextualSearchUi() -> CSManager#handleHideContextualSearch(). The extra layer of indirection does make it a bit harder to trace through the code, but maybe that's an okay sacrifice for having one class that documents and controls the sequence of internal states. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java (left): https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:441: // crbug.com/508354 Why is this TODO no longer neeeded? https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java (right): https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:261: // anymore. Can we remove this comment too? https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:353: * Handles Tap suppression by making a callback to either the handler's handleSuppressedTap or nit: #handleSuppressedTap() or #handleNonSuppressedTap() https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:355: * This should be called when the context is fully build (by gathering surrounding text nit: s/build/built
Thanks so much for the review Theresa! I'll bet it was one of the more complicated one's you've done, and I appreciate that work. I'll let you know when this is ready to be reviewed again, hopefully by early tomorrow. I plan to fix all the tests and get the bots green. I'll let you know. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java (right): https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:554: requestUpdate(); On 2017/04/25 17:30:41, Theresa wrote: > Is this fixing a pre-existing bug? Not a bug that we have an issue for, but one I discovered during testing of this CL. Probably best to write it up and land separately. I'll give that a try! https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchContext.java (right): https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchContext.java:165: * TODO(donnd): This method of finding the adjustment to the selection is unreliable! On 2017/04/25 17:30:41, Theresa wrote: > How unreliable? Is this something we need to fix before this patch makes it into > Stable? Dev? I think we need to fix this before Stable, but it's probably OK for Dev and Beta. I don't think I've seen it fail in mainstream cases, but it might in CJK especially if there are constructs in the language where the same word appears twice in a row (e.g. "I showed her her message") and the user taps between them. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchContext.java:176: && !TextUtils.isEmpty(mSurroundingText) && !TextUtils.isEmpty(selection)) { On 2017/04/25 17:30:41, Theresa wrote: > Let's do an early return instead to avoid one level of nesting/indentation. Funny, I had it that way but the early return was a not really visible since it's in the middle of a lot of code. But, there is some ugly wrapping here, so I'm happy to change it back. Done. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchContext.java:194: * @param startAdjust On 2017/04/25 17:30:41, Theresa wrote: > Finish JavaDoc Done. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateController.java (right): https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateController.java:45: * States can be "start states" which can be passed to #enter, or "transitional states" which On 2017/04/25 17:30:41, Theresa wrote: > nit s/#enter/#enter() Done. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateController.java:52: // destroyed. On 2017/04/25 17:30:41, Theresa wrote: > nit: Can these be JavaDoc so that IDEs show in-line help on hover? I didn't realize we could do that. Great idea! Done. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateController.java:77: // Start showing the Tap UI. Currently this means select the word tapped. On 2017/04/25 17:30:41, Theresa wrote: > nit: select the word tapped - > select the word that was tapped? Done. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateController.java:127: * @param reason the reason for the reset On 2017/04/25 17:30:41, Theresa wrote: > nit: The Chromium code base capitalize the start of parameter descriptions in > JavaDocs. > > We've discussed this before, right? Where did we land? Yes we have, and that is correct -- Chromium and Android use "normal" comments (with normal punctuation). For some reason I did a lot of these using the google3 java style. Fixed throughout this file. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateController.java:288: reset(StateChangeReason.BASE_PAGE_TAP); On 2017/04/25 17:30:41, Theresa wrote: > If we get another tap near by, we'll start another action (TAP_RECOGNIZED?) > rather than calling finishWorkingOn? That's right. I added a comment to that effect. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:89: // This setting is not critical: in practice it determines how long to wait after an invalid On 2017/04/25 17:30:41, Theresa wrote: > s/invalid for/invalid tap for Done. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:91: private static final int TAP_NEAR_PREVIOUS_DETECTION_DELAY_MS = 100; On 2017/04/25 17:30:42, Theresa wrote: > We should revisit this delay after we gather surroundings and decide suppression > before selection is established since that may take longer than 100ms. Actually I think that the instant the next tap is recognized we'll enter a new state, which will eventually take care of hiding or keeping the Bar open, and the wait associated with this value will end up being immaterial since it will be ingored. I think this setting just needs to be long enough for Blink's decisions before calling handleShowUnhandledTapUIIfNeeded (which probably are page-dependent), and short enough that the Bar goes away fairly quickly after a tap on non-text or whitespace -- we end up not getting notification in these cases (hence the timer). I updated the comment to better reflect what I've said here. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:498: hideContextualSearch(StateChangeReason.UNKNOWN); On 2017/04/25 17:30:42, Theresa wrote: > Should we fall back to a verbatim search rather than hiding the UI? I don't think so, I think the kinds of situations where this might happen are very unlikely and probably involve some kind of page navigation or internal code bug in which hiding the UX is best. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:644: if (!mInternalStateController.isStillWorkingOn(InternalState.RESOLVING)) return; On 2017/04/25 17:30:42, Theresa wrote: > On a really slow network, is it possible for something like this to happen? > > 1) Send resolution request > 2) User taps nearby to start another search, another resolution request is sent > 3) First resolution request comes in, the internal state is resolving so we > handle the resolution response even though it's for an old request I don't think this can happen because the native URLFetcher supports a single inflight model: the contextual_search_delegate calls URLFetcher#reset() which cancels pending fetches. The delegate synchronously calls reset when a new resolve starts allowing only one resolve in-flight at a time. However a bug in that code or the fetcher could cause a situation like you describe. When I was working on the design of the InternalStateController I thought about trying to handle cases like this. E.g. by having a serial number or timestamp on the context and processing only the most recent, aborting the rest. But I decided that would require some storage during async processes for the latest key, which doesn't seem to be available. Short of cases like a bug in the fetcher that would effectively be equivalent to this one-state model. I think we test this when we tap around a lot quickly. I updated the javadoc for this class with a brief description of this one-in-flight model. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1224: // TODO(donnd): Add support for this call from selectWordAroundCaret and add @Override On 2017/04/25 17:30:41, Theresa wrote: > Is this method being called yet? If not, should we pull it out for now? Good point, it's a placeholder for future work. Removed. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1225: public void selectWordAroundCaretAck(boolean didSelect, int startAdjust, int endAdjust) { On 2017/04/25 17:30:42, Theresa wrote: > Add a JavaDoc for this method Removed. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1280: if (mIsAccessibilityModeEnabled) return; On 2017/04/25 17:30:41, Theresa wrote: > If we return early because we're in accessibility mode, will the state handler > be left in InternalState.DECIDING_SUPPRESSION? Good question. I decided all calls should be checking this mode so it should never be in an inconsistent state (unless you can change the setting while CS is resolving). Manually tested, seems to work as-is. I'll try to add a test for this, since it would be good to auto-test. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1314: if (mIsAccessibilityModeEnabled) return; On 2017/04/25 17:30:42, Theresa wrote: > Same question about early return and leaving the controller in an invalid state > here. Seems to always leave the controller in IDLE. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1333: mInternalStateController.enter(InternalState.LONG_PRESS_RECOGNIZED); On 2017/04/25 17:30:41, Theresa wrote: > It may be worth documenting here what happens when we enter > long_press_recognized (and why we only "recognize" a long press but start > showing the ui for tap). > > I think this is what's happening, is my understanding correct? > > LONG_PRESS_RECOGNIZED -> transitionTo(InternalState.GATHERING_SURROUNDINGS) > though mechanisms in the internal state handler -> > CSInternalStateHandler#gatherSurroundingText() -> finish gathering, notify state > handler, transitionTo(InternalState.SHOWING_LONGPRESS_SEARCH) Yes, that's right. Part of why this is complicated is that we don't get the ACK for selectWordAroundCaret, instead we need to lurk here waiting for the selection to change. I'll add comments and a TODO with the same bug # so we can be sure to update all the places where the ACK would change code. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1418: boolean isTap = mSelectionController.getSelectionType() == SelectionType.TAP; On 2017/04/25 17:30:42, Theresa wrote: > nit: add a blank line before this one Done. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1440: if (baseWebContents != null && mPolicy.isTapSupported()) { On 2017/04/25 17:30:41, Theresa wrote: > In a follow up CL, let's revisit when we drop out if tap isn't supported. > Ideally we would stop processing as soon as possible to avoid gathering > surroundings, deciding suppression, etc. if we're going to end up dropping > everything because tap isn't supported. Added a TODO and bug for this. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1487: public void hideContextualSearchUi(StateChangeReason reason) { On 2017/04/25 17:30:41, Theresa wrote: > Moving this to a private internal implementation certainly helps a lot with the > issue of knowing whether to call hideContextualSearch() or > hideContextualSearchUi()! > > It is interesting, though, that rather than one method to hide the UI we now > have: > > CSManager#hideContextualSearch() -> > ContextualSearchInternalStateController#reset() -> > CSInternalStateHandler#hideContextualSearchUi() -> > CSManager#handleHideContextualSearch(). > > The extra layer of indirection does make it a bit harder to trace through the > code, but maybe that's an okay sacrifice for having one class that documents and > controls the sequence of internal states. Agreed: I have mixed feelings about the refactoring. cons: Mostly it makes everything somewhat more complicated. Pros: It puts the overall flow in one place and connects the dots on those async calls. Let's keep thinking about this and whether it can be improved, e.g. showContextualSearchTapUi is connected to InternalState.SHOW_FULL_TAP_UI but that's not clearly stated -- maybe I should update the handler interface to make that clear (done). Other ideas? https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java (left): https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:441: // crbug.com/508354 On 2017/04/25 17:30:42, Theresa wrote: > Why is this TODO no longer neeeded? Added a check in handleSearchTermResolutionResponse. Will close the bug. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java (right): https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:261: // anymore. On 2017/04/25 17:30:42, Theresa wrote: > Can we remove this comment too? Done. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:353: * Handles Tap suppression by making a callback to either the handler's handleSuppressedTap or On 2017/04/25 17:30:42, Theresa wrote: > nit: #handleSuppressedTap() or #handleNonSuppressedTap() Done. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:355: * This should be called when the context is fully build (by gathering surrounding text On 2017/04/25 17:30:42, Theresa wrote: > nit: s/build/built Done.
Theresa, PTAL. Here's some notes about today's changes (below). https://codereview.chromium.org/2703473002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateHandler.java (right): https://codereview.chromium.org/2703473002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateHandler.java:16: * {@See ContextualSearchInternalStateController#InternalState#IDLE}. Added these @See annotations. https://codereview.chromium.org/2703473002/diff/280001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java (right): https://codereview.chromium.org/2703473002/diff/280001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2875: public void testAccesibilityMode() throws InterruptedException, TimeoutException { Added this test for accessibility mode, and a few tests that tap start/end sequences match. https://codereview.chromium.org/2703473002/diff/280001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchTapEventTest.java (right): https://codereview.chromium.org/2703473002/diff/280001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchTapEventTest.java:203: private void mockLongpressText(String text) { Renamed this to reflect what it really does. https://codereview.chromium.org/2703473002/diff/280001/chrome/browser/android... File chrome/browser/android/contextualsearch/contextual_search_context.cc (right): https://codereview.chromium.org/2703473002/diff/280001/chrome/browser/android... chrome/browser/android/contextualsearch/contextual_search_context.cc:68: DCHECK(start_offset + j_start_adjust >= 0); Added this bounds checking.
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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Theresa, PTAL. Thanks so much!
https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchContext.java (right): https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchContext.java:165: * TODO(donnd): This method of finding the adjustment to the selection is unreliable! On 2017/04/25 22:35:36, Donn Denman wrote: > On 2017/04/25 17:30:41, Theresa wrote: > > How unreliable? Is this something we need to fix before this patch makes it > into > > Stable? Dev? > > I think we need to fix this before Stable, but it's probably OK for Dev and > Beta. I don't think I've seen it fail in mainstream cases, but it might in CJK > especially if there are constructs in the language where the same word appears > twice in a row (e.g. "I showed her her message") and the user taps between them. Can we make crbug.com/435778 release-blocking to make sure this doesn't fall off the radar for M60? https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateController.java (right): https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateController.java:127: * @param reason the reason for the reset On 2017/04/25 22:35:37, Donn Denman wrote: > On 2017/04/25 17:30:41, Theresa wrote: > > nit: The Chromium code base capitalize the start of parameter descriptions in > > JavaDocs. > > > > We've discussed this before, right? Where did we land? > > Yes we have, and that is correct -- Chromium and Android use "normal" comments > (with normal punctuation). For some reason I did a lot of these using the > google3 java style. > > Fixed throughout this file. Thanks! https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:498: hideContextualSearch(StateChangeReason.UNKNOWN); On 2017/04/25 22:35:37, Donn Denman wrote: > On 2017/04/25 17:30:42, Theresa wrote: > > Should we fall back to a verbatim search rather than hiding the UI? > > I don't think so, I think the kinds of situations where this might happen are > very unlikely and probably involve some kind of page navigation or internal code > bug in which hiding the UX is best. Acknowledged. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:644: if (!mInternalStateController.isStillWorkingOn(InternalState.RESOLVING)) return; On 2017/04/25 22:35:37, Donn Denman wrote: > On 2017/04/25 17:30:42, Theresa wrote: > > On a really slow network, is it possible for something like this to happen? > > > > 1) Send resolution request > > 2) User taps nearby to start another search, another resolution request is > sent > > 3) First resolution request comes in, the internal state is resolving so we > > handle the resolution response even though it's for an old request > > I don't think this can happen because the native URLFetcher supports a single > inflight model: the contextual_search_delegate calls URLFetcher#reset() which > cancels pending fetches. The delegate synchronously calls reset when a new > resolve starts allowing only one resolve in-flight at a time. However a bug in > that code or the fetcher could cause a situation like you describe. > > When I was working on the design of the InternalStateController I thought about > trying to handle cases like this. E.g. by having a serial number or timestamp > on the context and processing only the most recent, aborting the rest. But I > decided that would require some storage during async processes for the latest > key, which doesn't seem to be available. Short of cases like a bug in the > fetcher that would effectively be equivalent to this one-state model. > > I think we test this when we tap around a lot quickly. > > I updated the javadoc for this class with a brief description of this > one-in-flight model. I think it makes sense for native to handle -- thank you for the explanation. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1487: public void hideContextualSearchUi(StateChangeReason reason) { On 2017/04/25 22:35:37, Donn Denman wrote: > On 2017/04/25 17:30:41, Theresa wrote: > > Moving this to a private internal implementation certainly helps a lot with > the > > issue of knowing whether to call hideContextualSearch() or > > hideContextualSearchUi()! > > > > It is interesting, though, that rather than one method to hide the UI we now > > have: > > > > CSManager#hideContextualSearch() -> > > ContextualSearchInternalStateController#reset() -> > > CSInternalStateHandler#hideContextualSearchUi() -> > > CSManager#handleHideContextualSearch(). > > > > The extra layer of indirection does make it a bit harder to trace through the > > code, but maybe that's an okay sacrifice for having one class that documents > and > > controls the sequence of internal states. > > Agreed: I have mixed feelings about the refactoring. cons: Mostly it makes > everything somewhat more complicated. Pros: It puts the overall flow in one > place and connects the dots on those async calls. > > Let's keep thinking about this and whether it can be improved, e.g. > showContextualSearchTapUi is connected to > InternalState.SHOW_FULL_TAP_UI but that's not clearly stated -- maybe I should > update the handler interface to make that clear (done). Other ideas? > I think refactoring CSManager to pull out some of the interface implementations and generally make it simpler would help a lot. For the indirection with hiding, can we move the handleHideContextualSearch() code to this method to remove one link in the chain? We may also consider doing something similar for showContextualSearch() and some of the other methods. I'll keep thinking on other ideas. The extra complexity is unfortunate but the pros of documenting our async flow, being able to easily change the flow, and allowing surroundings to be gathered before selection is established out-weigh the con in my opinion. https://codereview.chromium.org/2703473002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateHandler.java (right): https://codereview.chromium.org/2703473002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateHandler.java:16: * {@See ContextualSearchInternalStateController#InternalState#IDLE}. On 2017/04/26 03:43:54, Donn Denman wrote: > Added these @See annotations. Acknowledged. https://codereview.chromium.org/2703473002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703473002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1386: private void showSelectionAsSearchInBar() { findbugs pointed out that this method is never called https://codereview.chromium.org/2703473002/diff/280001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateControllerWrapper.java (right): https://codereview.chromium.org/2703473002/diff/280001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateControllerWrapper.java:12: class ContextualSearchInternalStateControllerWrapper nit: JavaDoc?
Theresa, PTAL. Thanks for the review! https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchContext.java (right): https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchContext.java:165: * TODO(donnd): This method of finding the adjustment to the selection is unreliable! On 2017/04/26 18:01:14, Theresa wrote: > On 2017/04/25 22:35:36, Donn Denman wrote: > > On 2017/04/25 17:30:41, Theresa wrote: > > > How unreliable? Is this something we need to fix before this patch makes it > > into > > > Stable? Dev? > > > > I think we need to fix this before Stable, but it's probably OK for Dev and > > Beta. I don't think I've seen it fail in mainstream cases, but it might in > CJK > > especially if there are constructs in the language where the same word appears > > twice in a row (e.g. "I showed her her message") and the user taps between > them. > > Can we make crbug.com/435778 release-blocking to make sure this doesn't fall off > the radar for M60? Done. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateController.java (right): https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateController.java:127: * @param reason the reason for the reset On 2017/04/26 18:01:14, Theresa wrote: > On 2017/04/25 22:35:37, Donn Denman wrote: > > On 2017/04/25 17:30:41, Theresa wrote: > > > nit: The Chromium code base capitalize the start of parameter descriptions > in > > > JavaDocs. > > > > > > We've discussed this before, right? Where did we land? > > > > Yes we have, and that is correct -- Chromium and Android use "normal" comments > > (with normal punctuation). For some reason I did a lot of these using the > > google3 java style. > > > > Fixed throughout this file. > > Thanks! Acknowledged. https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:644: if (!mInternalStateController.isStillWorkingOn(InternalState.RESOLVING)) return; On 2017/04/26 18:01:14, Theresa wrote: > On 2017/04/25 22:35:37, Donn Denman wrote: > > On 2017/04/25 17:30:42, Theresa wrote: > > > On a really slow network, is it possible for something like this to happen? > > > > > > 1) Send resolution request > > > 2) User taps nearby to start another search, another resolution request is > > sent > > > 3) First resolution request comes in, the internal state is resolving so we > > > handle the resolution response even though it's for an old request > > > > I don't think this can happen because the native URLFetcher supports a single > > inflight model: the contextual_search_delegate calls URLFetcher#reset() which > > cancels pending fetches. The delegate synchronously calls reset when a new > > resolve starts allowing only one resolve in-flight at a time. However a bug > in > > that code or the fetcher could cause a situation like you describe. > > > > When I was working on the design of the InternalStateController I thought > about > > trying to handle cases like this. E.g. by having a serial number or timestamp > > on the context and processing only the most recent, aborting the rest. But I > > decided that would require some storage during async processes for the latest > > key, which doesn't seem to be available. Short of cases like a bug in the > > fetcher that would effectively be equivalent to this one-state model. > > > > I think we test this when we tap around a lot quickly. > > > > I updated the javadoc for this class with a brief description of this > > one-in-flight model. > > I think it makes sense for native to handle -- thank you for the explanation. Sure! https://codereview.chromium.org/2703473002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1487: public void hideContextualSearchUi(StateChangeReason reason) { On 2017/04/26 18:01:14, Theresa wrote: > On 2017/04/25 22:35:37, Donn Denman wrote: > > On 2017/04/25 17:30:41, Theresa wrote: > > > Moving this to a private internal implementation certainly helps a lot with > > the > > > issue of knowing whether to call hideContextualSearch() or > > > hideContextualSearchUi()! > > > > > > It is interesting, though, that rather than one method to hide the UI we now > > > have: > > > > > > CSManager#hideContextualSearch() -> > > > ContextualSearchInternalStateController#reset() -> > > > CSInternalStateHandler#hideContextualSearchUi() -> > > > CSManager#handleHideContextualSearch(). > > > > > > The extra layer of indirection does make it a bit harder to trace through > the > > > code, but maybe that's an okay sacrifice for having one class that documents > > and > > > controls the sequence of internal states. > > > > Agreed: I have mixed feelings about the refactoring. cons: Mostly it makes > > everything somewhat more complicated. Pros: It puts the overall flow in one > > place and connects the dots on those async calls. > > > > Let's keep thinking about this and whether it can be improved, e.g. > > showContextualSearchTapUi is connected to > > InternalState.SHOW_FULL_TAP_UI but that's not clearly stated -- maybe I should > > update the handler interface to make that clear (done). Other ideas? > > > > I think refactoring CSManager to pull out some of the interface implementations > and generally make it simpler would help a lot. > > For the indirection with hiding, can we move the handleHideContextualSearch() > code to this method to remove one link in the chain? We may also consider doing > something similar for showContextualSearch() and some of the other methods. Done. I also moved hideContextualSearchUi up to the top of this section, since it's special due to handling of a reset() / moving to IDLE. Moving show is not as easy because we call it for both long-press and tap here. > > I'll keep thinking on other ideas. Thanks! > > The extra complexity is unfortunate but the pros of documenting our async flow, > being able to easily change the flow, and allowing surroundings to be gathered > before selection is established out-weigh the con in my opinion. Good to know. https://codereview.chromium.org/2703473002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/2703473002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1386: private void showSelectionAsSearchInBar() { On 2017/04/26 18:01:15, Theresa wrote: > findbugs pointed out that this method is never called Removed, thanks for flagging. https://codereview.chromium.org/2703473002/diff/280001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateControllerWrapper.java (right): https://codereview.chromium.org/2703473002/diff/280001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateControllerWrapper.java:12: class ContextualSearchInternalStateControllerWrapper On 2017/04/26 18:01:15, Theresa wrote: > nit: JavaDoc? Done for the class, constructor, and public methods. https://codereview.chromium.org/2703473002/diff/280001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateControllerWrapper.java:26: ContextualSearchInternalStateControllerWrapper( Also realized this constructor is only called from this class so I made it private.
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...
lgtm
donnd@chromium.org changed reviewers: + tedchoc@chromium.org
Ted, PTAL at ChromeActivity. Thanks everyone!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/26 20:38:26, Donn Denman wrote: > Ted, PTAL at ChromeActivity. > > Thanks everyone! ChromeActivity.java - 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": 300001, "attempt_start_ts": 1493250941547470,
"parent_rev": "19045842e1eff8063bd5aca05c8a94aa2035f7a5", "commit_rev":
"e78147e28cc531df52ba73e0efe49d0401d28f6b"}
Message was sent while issue was closed.
Description was changed from
==========
[TTS] Extract tapped text before showing UI.
Reworks Tap gesture recognition and processing to use a new Internal
State Controller. This controller drives the ContextualSearchManager
through its sequence of state transitions. Now the manager is just
a library of operations, and the overall flow of control is handled
exclusively by the new state controller.
Now text is extracted before showing any UI. This is needed so we
can apply ML to decide when to show the UI.
The Java Context now tracks a small section of content of the Context
around the Selection. Context now has a hack to figure out how the
content changed when a word is selected with SelectWordAroundCaret.
This should be fixed soon by adding an ACK to SelectWordAroundCaret.
Stopped using the GestureStateListener#onSingleTap override so we
should be able to remove that method from the interface in a future
CL.
The SelectionController no longer needs to have a delay for a possible
invalid tap because there's a state that waits for a possible tap
nearby a previous tap.
BUG=692824
==========
to
==========
[TTS] Extract tapped text before showing UI.
Reworks Tap gesture recognition and processing to use a new Internal
State Controller. This controller drives the ContextualSearchManager
through its sequence of state transitions. Now the manager is just
a library of operations, and the overall flow of control is handled
exclusively by the new state controller.
Now text is extracted before showing any UI. This is needed so we
can apply ML to decide when to show the UI.
The Java Context now tracks a small section of content of the Context
around the Selection. Context now has a hack to figure out how the
content changed when a word is selected with SelectWordAroundCaret.
This should be fixed soon by adding an ACK to SelectWordAroundCaret.
Stopped using the GestureStateListener#onSingleTap override so we
should be able to remove that method from the interface in a future
CL.
The SelectionController no longer needs to have a delay for a possible
invalid tap because there's a state that waits for a possible tap
nearby a previous tap.
BUG=692824
Review-Url: https://codereview.chromium.org/2703473002
Cr-Commit-Position: refs/heads/master@{#467522}
Committed:
https://chromium.googlesource.com/chromium/src/+/e78147e28cc531df52ba73e0efe4...
==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as https://chromium.googlesource.com/chromium/src/+/e78147e28cc531df52ba73e0efe4... |
