Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2211353002:
[TTS] Gather surrounding text on Tap before any UX. (Closed)
|
Created:
4 years, 4 months ago by Donn Denman Modified:
4 years, 3 months ago CC:
chromium-reviews, twellington+watch_chromium.org, donnd+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[TTS] Gather surrounding text on Tap before any UX.
Extract the text tapped on to use as a signal in Tap Suppression.
The text is extracted before any UX is displayed in order to allow the
tap to be totally ignored when appropriate. Feeding the surrounding
text into the logic of TTS will be done separately.
This adds several files that are part of the 2016-refactoring.
See crbug.com/624609 and go/cs-refactoring-2016.
This CL is part of the refactoring-2016 effort, see go/cs-refactoring-2016
for details.
BUG=634136, 624609
Committed: https://crrev.com/3f11e42880b1b7b6608e2d57fa538552318367ae
Cr-Commit-Position: refs/heads/master@{#418464}
Patch Set 1 #Patch Set 2 : [TTS] Gather surrounding text on Tap before any UX. #Patch Set 3 : Split usage of the Tapped text from the SearchAction into a separate CL. #
Total comments: 43
Patch Set 4 : Addressed Theresa's initial comments. #Patch Set 5 : Changed passing the text sample to java -- now it's by Java request only (currently unused). #
Total comments: 2
Patch Set 6 : Removed java accessors to the tapped text, added simple public native accessors. #Patch Set 7 : Simplified the results for the native accessors to the context. Removed print statements. #
Total comments: 26
Patch Set 8 : Reworked the native accessors to tapped text and the associated unit tests. Destroying Actions on … #
Total comments: 2
Patch Set 9 : Updated a comment and added a TODO. #Patch Set 10 : Rebase and cleanup minor build issues. #Patch Set 11 : Just another rebase. #Patch Set 12 : Updated instrumentation tests to remove tapBasePageToClosePanel and just call closePanel instead. #
Total comments: 4
Patch Set 13 : Removed unused code and consolidated two functions, updated comments. #Messages
Total messages: 45 (18 generated)
Description was changed from ========== [TTS] Gather surrounding text on Tap before any UX. BUG=634136 ========== to ========== [TTS] Gather surrounding text on Tap before any UX. Extract the text tapped on to use as a signal in Tap Suppression. The text is extracted before any UX is displayed in order to allow the tap to be totally ignored when appropriate. Feeding the surrounding text into the logic of TTS will be done separately. This adds several files that are part of the 2016-refactoring. See crbug.com/624609 and go/cs-refactoring-2016. This CL is part of the refactoring-2016 effort, see go/cs-refactoring-2016 for details. BUG=634136, 624609 ==========
donnd@chromium.org changed reviewers: + twellington@chromium.org
Theresa, This is ready for a first quick-look. Thanks! BTW, I will upload the base CL with the full refactoring in case you would like to see what's changed versus Pedro's work.
A few questions, I'm sure I'll have more. Overall, looks like a big step toward the refactoring. https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java (right): https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:30: public class ContextualSearchSelectionController implements SearchGestureHost { Longer term this class will become SearchGestureController? https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:131: if (mHasIdentifiedUnhandledTap) createSearchAction(); This is happening before the selection is established, correct? https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:357: mHasIdentifiedUnhandledTap = false; nit: reset mTapTimeNanos here too? https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:415: // TODO(donnd): use the supplied searchAction! The plan is to move these suppressions into the search action rather than calculting suppression here? https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:486: ContentViewCore baseCvc = getBaseContentView(); I believe we're trying to remove CVC dependencies (see crbug.com/626848 and ask mdjones@) https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/ResolvedSearchAction.java (right): https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/ResolvedSearchAction.java:15: public class ResolvedSearchAction extends SearchAction { Right now this class is just being used to extract surrounding context for a tap action before issuing the resolution request to determine if we want to suppress the tap? Longer term it will actually do the resolution request & search request (or one single request)? https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java (right): https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java:12: * Represents an abstract action to do a Contextual Search, and supports native C++ functiionality. nit: s/functiionality/functionality https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java:77: return mSurroundingTextSample.substring(mFocusedWordEnd - mSurroundingTextSampleStart); Should this just be mFocusedWordEnd? e.g. mFocusedWordEnd = 10, mSourrindTextSampleStart = 7, so I get a substring starting at 3? Wouldn't we just want a substring starting at 10? https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchActionListener.java (right): https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchActionListener.java:13: * Indicates that the Context is ready. "Context" is a pre-existing Android class. This needs some more explanation e.g. the Contextual Search Context, which includes the selected word and surrounding text, ... https://codereview.chromium.org/2211353002/diff/40001/chrome/browser/android/... File chrome/browser/android/contextualsearch/search_action.cc (right): https://codereview.chromium.org/2211353002/diff/40001/chrome/browser/android/... chrome/browser/android/contextualsearch/search_action.cc:185: // TODO(pedrosimonetti): should not include CJK characters! Let's be sure to get some manual test coverage on CJK to make sure we're not breaking anything there. https://codereview.chromium.org/2211353002/diff/40001/chrome/browser/android/... chrome/browser/android/contextualsearch/search_action.cc:187: if ((char_code >= 11264 && char_code <= 55295) || Can we comment on whta these are as well?
A bigger qusetion I have is how this fits in with the planned C++ signal mixer? In the current plan SearchAction is responsible for suppression, so maybe we would hook into search_action.cc? If we're hooking in suppression on the native side via the signal mixer, does surrounding context still need to go to Java, or can we determine whether or not to suppress natively and just make the Java side aware that the action was suppressed?
donnd@chromium.org changed reviewers: + pedrosimonetti@chromium.org
Pedro, adding you as a reviewer, feel free to review as much or little as you wish. Theresa, thanks for the review! PTAL in the next day or two. https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java (right): https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:30: public class ContextualSearchSelectionController implements SearchGestureHost { On 2016/08/16 15:41:49, Theresa Wellington wrote: > Longer term this class will become SearchGestureController? I believe this class becomes the SearchGestureRecognizer (see Pedro's doc at https://docs.google.com/document/d/1zHwDtjUPmf9gMy6a4i1eyes2RwdJIfx1nKC-PLGhk...). But this class has grown beyond its original charter of recognizing gestures, so those other roles move into the new classes, mostly the SearchActionController I think. Pedro just agreed to be a reviewer so we can get his perspective on this too. https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:131: if (mHasIdentifiedUnhandledTap) createSearchAction(); On 2016/08/16 15:41:49, Theresa Wellington wrote: > This is happening before the selection is established, correct? That's right. https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:357: mHasIdentifiedUnhandledTap = false; On 2016/08/16 15:41:49, Theresa Wellington wrote: > nit: reset mTapTimeNanos here too? Good idea, done. https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:415: // TODO(donnd): use the supplied searchAction! On 2016/08/16 15:41:49, Theresa Wellington wrote: > The plan is to move these suppressions into the search action rather than > calculting suppression here? That's right. https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:486: ContentViewCore baseCvc = getBaseContentView(); On 2016/08/16 15:41:49, Theresa Wellington wrote: > I believe we're trying to remove CVC dependencies (see crbug.com/626848 and ask > mdjones@) Reworked this to not use a CVC, and added a TODO (see note below). This made me notice we're not checking for null in the caller, so added some safeguarding there. https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:502: ContentViewCore getBaseContentView() { Added a TODO here to stop using CVC. https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/ResolvedSearchAction.java (right): https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/ResolvedSearchAction.java:15: public class ResolvedSearchAction extends SearchAction { On 2016/08/16 15:41:49, Theresa Wellington wrote: > Right now this class is just being used to extract surrounding context for a tap > action before issuing the resolution request to determine if we want to suppress > the tap? Longer term it will actually do the resolution request & search request > (or one single request)? > Correct. Added your nice description to the header comment. https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java (right): https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java:12: * Represents an abstract action to do a Contextual Search, and supports native C++ functiionality. On 2016/08/16 15:41:49, Theresa Wellington wrote: > nit: s/functiionality/functionality Done. https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java:77: return mSurroundingTextSample.substring(mFocusedWordEnd - mSurroundingTextSampleStart); On 2016/08/16 15:41:49, Theresa Wellington wrote: > Should this just be mFocusedWordEnd? > > e.g. mFocusedWordEnd = 10, mSourrindTextSampleStart = 7, so I get a substring > starting at 3? Wouldn't we just want a substring starting at 10? Oh, this is confusing -- these offsets are within the original full surrounding text, not within the sample! Pedro, is there good reason why these offsets shouldn't be relative to the sample? I added a TODO to change this. But that's probably best done in the C++ code that returns the offsets. Does that sound right to both of you? https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchActionListener.java (right): https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchActionListener.java:13: * Indicates that the Context is ready. On 2016/08/16 15:41:49, Theresa Wellington wrote: > "Context" is a pre-existing Android class. This needs some more explanation e.g. > the Contextual Search Context, which includes the selected word and surrounding > text, ... Done. https://codereview.chromium.org/2211353002/diff/40001/chrome/browser/android/... File chrome/browser/android/contextualsearch/search_action.cc (right): https://codereview.chromium.org/2211353002/diff/40001/chrome/browser/android/... chrome/browser/android/contextualsearch/search_action.cc:185: // TODO(pedrosimonetti): should not include CJK characters! On 2016/08/16 15:41:49, Theresa Wellington wrote: > Let's be sure to get some manual test coverage on CJK to make sure we're not > breaking anything there. Acknowledged. https://codereview.chromium.org/2211353002/diff/40001/chrome/browser/android/... chrome/browser/android/contextualsearch/search_action.cc:185: // TODO(pedrosimonetti): should not include CJK characters! On 2016/08/16 15:41:49, Theresa Wellington wrote: > Let's be sure to get some manual test coverage on CJK to make sure we're not > breaking anything there. Pedro, why shouldn't CJK characters be included here? Maybe you meant some other CJK items like punctuation? https://codereview.chromium.org/2211353002/diff/40001/chrome/browser/android/... chrome/browser/android/contextualsearch/search_action.cc:187: if ((char_code >= 11264 && char_code <= 55295) || On 2016/08/16 15:41:49, Theresa Wellington wrote: > Can we comment on whta these are as well? Done.
On 2016/08/16 16:09:47, Theresa Wellington wrote: > A bigger qusetion I have is how this fits in with the planned C++ signal mixer? > In the current plan SearchAction is responsible for suppression, so maybe we > would hook into search_action.cc? > > If we're hooking in suppression on the native side via the signal mixer, does > surrounding context still need to go to Java, or can we determine whether or not > to suppress natively and just make the Java side aware that the action was > suppressed? Theresa, I forgot to address your main question, in part because I don't think I fully know yet. For the signal mixer I just had an email interaction with marq@ who points out that the best way to share code with iOS is to put C++ code into component/, so I'm thinking the bulk of the signal mixer code belongs there. I think gathering various signals is somewhat platform-dependent and will need to come from several sources and languages. The SearchAction does provide the basis for surrounding text signals useful for "searchyness" which itself should probably be a few independent signals such as word-length and capitalization, etc. I'll work on a separate dependent CL for the signal mixer and probably another that gathers various signals and assembles them. Will CC you on both.
On 2016/08/17 20:58:03, Donn Denman wrote: .... > I think gathering various signals is somewhat platform-dependent > and will need to come from several sources and languages. I agree that a number of signals are platform-dependent e.g. recent scroll, where the tap on the screen occurs, etc. > The SearchAction does provide the basis for surrounding text signals useful for > "searchyness" which itself should probably be a > few independent signals such as word-length and capitalization, etc. The signals that come from the text itself, however, shouldn't be platform-dependent (right?) I think the main win of this CL is that it brings surrounding context in before we make a decision to suppress. Longer term, I think we want native code analyzing the surrounding text and determining "searchyness" since we will have to feed it to the C++ mixer anyway. Doing the surrounding text analysis natively also opens the door to sharing that with iOS. With this patchset, we're actually sending the surrounding text to Java twice (maybe there's a way around that? Currently we also send it to ContextualSearchManager#onIcingSelectionAvailable()). If long-term we think want to do surrounding context analysis in Java, then let's push this through and worry about sending multiple times later. If, however, we think long-term we want to analyze surrounding text in the native code, then what is our win for pushing this through now? Are there experiments blocked on getting this landed that we want to get rolling?
On 2016/08/18 18:29:32, Theresa Wellington wrote: > On 2016/08/17 20:58:03, Donn Denman wrote: > .... > > I think gathering various signals is somewhat platform-dependent > > and will need to come from several sources and languages. > > I agree that a number of signals are platform-dependent e.g. recent scroll, > where the tap on the screen occurs, etc. > > > > The SearchAction does provide the basis for surrounding text signals useful > for > > "searchyness" which itself should probably be a > > few independent signals such as word-length and capitalization, etc. > > The signals that come from the text itself, however, shouldn't be > platform-dependent (right?) > > I think the main win of this CL is that it brings surrounding context in before > we make a decision to > suppress. Longer term, I think we want native code analyzing the surrounding > text and > determining "searchyness" since we will have to feed it to the C++ mixer anyway. > Doing the surrounding text > analysis natively also opens the door to sharing that with iOS. > > With this patchset, we're actually sending the surrounding text to Java twice > (maybe there's a way around that? Currently we also send it to > ContextualSearchManager#onIcingSelectionAvailable()). If long-term we think want > to do surrounding context analysis in Java, then let's push this through and > worry about sending multiple times later. If, however, we think long-term we > want to analyze surrounding text in the native code, then what is our win for > pushing this through now? Are there experiments blocked on getting this landed > that we want to get rolling? I think we'll want to analyze surrounding text in the native code. I'm working on extending the existing experiment to use an additional searchyness signal that this CL enables. My plan is to put as much of that code into the /components/contextual_search/ as possible. I think that can be done more easily with this CL in place. When I land native access to the context I'll undo the bit that sends it to Java as part of that work. Landing this CL also gets us one slice of the refactoring and experience with the process of landing it in slices and merging/updating.
https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java (right): https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:30: public class ContextualSearchSelectionController implements SearchGestureHost { On 2016/08/17 04:35:21, Donn Denman wrote: > On 2016/08/16 15:41:49, Theresa Wellington wrote: > > Longer term this class will become SearchGestureController? > > I believe this class becomes the SearchGestureRecognizer (see Pedro's doc at > https://docs.google.com/document/d/1zHwDtjUPmf9gMy6a4i1eyes2RwdJIfx1nKC-PLGhk...). > But this class has grown beyond its original charter of recognizing gestures, > so those other roles move into the new classes, mostly the > SearchActionController I think. > > Pedro just agreed to be a reviewer so we can get his perspective on this too. Yes, this class is doing lots of things, and ideally things would be separated in different classes. The main purpose of this class is to recognize a selection caused by a "search gesture", so eventually this would become the SearchGestureRecognizer class. https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:131: if (mHasIdentifiedUnhandledTap) createSearchAction(); Not sure this is the right place to call createAction(). An action should be created when *any* search gesture is detected. Similarly, an existing action should be destroyed when *any* other search gesture is detected, when the existing action is dismissed by a dismissal gesture, or when the Panel gets closed. Now, we are creating a search action when the tap gesture is detected, but we are not creating a new gesture when the long-press gesture is detected. Also, it seems we're not destroying the actions in all cases we should, like when a new gesture is detected, for example. https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:430: mHandler.handleSuppressedTap(); Should a suppressed gesture cause the action to be destroyed? https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:462: destroySearchAction(); Why are we destroying the action here? https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:486: ContentViewCore baseCvc = getBaseContentView(); I'd call this baseContentViewCore or simply contentViewCore (instead of abbreviating it), as it's being done elsewhere in the codebase. https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/ResolvedSearchAction.java (right): https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/ResolvedSearchAction.java:30: updateState(host); Why is updateState() being exposed? I think it should be kept private, given its updating the internal state of the super class. https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java (right): https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java:77: return mSurroundingTextSample.substring(mFocusedWordEnd - mSurroundingTextSampleStart); On 2016/08/17 04:35:22, Donn Denman wrote: > On 2016/08/16 15:41:49, Theresa Wellington wrote: > > Should this just be mFocusedWordEnd? > > > > e.g. mFocusedWordEnd = 10, mSourrindTextSampleStart = 7, so I get a substring > > starting at 3? Wouldn't we just want a substring starting at 10? > > Oh, this is confusing -- these offsets are within the original full surrounding > text, not within the sample! > > Pedro, is there good reason why these offsets shouldn't be relative to the > sample? > > I added a TODO to change this. But that's probably best done in the C++ code > that returns the offsets. > > Does that sound right to both of you? Goldmine will send us offsets relative to the whole string, not just the sample. Also Goldmine, has *inclusive* start offsets and *exclusive* end offsets, represented by the [start, end) interval, which means: start <= offset < end. For consistency reasons, and to avoid confusion, all indexes/intervals are considered to be relative to the whole string, with *inclusive* start offsets and *exclusive* end offsets. Subtracting mSourrindTextSampleStart from a index means converting it to sample based, as opposed to global based (based on the whole string). Global position (relative to whole string): mFocusedWordEnd Local position (relative to the sample string): mFocusedWordEnd - mSurroundingTextSampleStart https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java:163: protected void updateState(SearchGestureHost host) { I think this should be kept as private, as stated elsewhere. https://codereview.chromium.org/2211353002/diff/40001/chrome/browser/android/... File chrome/browser/android/contextualsearch/search_action.cc (right): https://codereview.chromium.org/2211353002/diff/40001/chrome/browser/android/... chrome/browser/android/contextualsearch/search_action.cc:185: // TODO(pedrosimonetti): should not include CJK characters! On 2016/08/17 04:35:22, Donn Denman wrote: > On 2016/08/16 15:41:49, Theresa Wellington wrote: > > Let's be sure to get some manual test coverage on CJK to make sure we're not > > breaking anything there. > > Pedro, why shouldn't CJK characters be included here? Maybe you meant some > other CJK items like punctuation? If we include all CJK charactes, as we are doing, then it will select all characters until it finds a non-character, like space or punctuation. This is a problem because our resolution engine will "expand" words when resolving, but not "shrink". This means resolution will be broken for CJK languages. Instead, the code will select chunks of characters that not necessarily translate to actual words. Example: "San" might be resolved to "San Francisco", but "San Francisco plus some extra characters" won't be resolved to "San Francisco".
Theresa and Pedro, PTAL at your leisure. I plan to land this after the cut on Thursday. I changed the way the surrounding text is handled. Now it stays in native code until java requests part of it, a word or a subsection. https://chromiumcodereview.appspot.com/2211353002/diff/40001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java (right): https://chromiumcodereview.appspot.com/2211353002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:30: public class ContextualSearchSelectionController implements SearchGestureHost { On 2016/08/22 20:54:17, pedro (no code reviews) wrote: > On 2016/08/17 04:35:21, Donn Denman wrote: > > On 2016/08/16 15:41:49, Theresa Wellington wrote: > > > Longer term this class will become SearchGestureController? > > > > I believe this class becomes the SearchGestureRecognizer (see Pedro's doc at > > > https://docs.google.com/document/d/1zHwDtjUPmf9gMy6a4i1eyes2RwdJIfx1nKC-PLGhk...). > > But this class has grown beyond its original charter of recognizing gestures, > > so those other roles move into the new classes, mostly the > > SearchActionController I think. > > > > Pedro just agreed to be a reviewer so we can get his perspective on this too. > > Yes, this class is doing lots of things, and ideally things would be separated > in different classes. The main purpose of this class is to recognize a selection > caused by a "search gesture", so eventually this would become the > SearchGestureRecognizer class. Acknowledged. https://chromiumcodereview.appspot.com/2211353002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:131: if (mHasIdentifiedUnhandledTap) createSearchAction(); On 2016/08/22 20:54:17, pedro (no code reviews) wrote: > Not sure this is the right place to call createAction(). > > An action should be created when *any* search gesture is detected. > > Similarly, an existing action should be destroyed when *any* other search > gesture is detected, when the existing action is dismissed by a dismissal > gesture, or when the Panel gets closed. > > Now, we are creating a search action when the tap gesture is detected, but we > are not creating a new gesture when the long-press gesture is detected. > > Also, it seems we're not destroying the actions in all cases we should, like > when a new gesture is detected, for example. In this initial integration of the refactoring we're only using the SearchAction to get the tapped-text for searchyness evaluation, not for all searches, so that's why we only make one on Tap. I'll make sure that any previous one is destroyed in createSearchAction(). Thanks for catching that! https://chromiumcodereview.appspot.com/2211353002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:430: mHandler.handleSuppressedTap(); On 2016/08/22 20:54:17, pedro (no code reviews) wrote: > Should a suppressed gesture cause the action to be destroyed? I think it's simplest to just destroy the search action on reset or when creating a new one. https://chromiumcodereview.appspot.com/2211353002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:462: destroySearchAction(); On 2016/08/22 20:54:17, pedro (no code reviews) wrote: > Why are we destroying the action here? I guess we don't need to destroy the action, so removed this override. https://chromiumcodereview.appspot.com/2211353002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:486: ContentViewCore baseCvc = getBaseContentView(); On 2016/08/22 20:54:17, pedro (no code reviews) wrote: > I'd call this baseContentViewCore or simply contentViewCore (instead of > abbreviating it), as it's being done elsewhere in the codebase. Done -- code changed to not use CVC at all. https://chromiumcodereview.appspot.com/2211353002/diff/40001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/ResolvedSearchAction.java (right): https://chromiumcodereview.appspot.com/2211353002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/ResolvedSearchAction.java:30: updateState(host); On 2016/08/22 20:54:17, pedro (no code reviews) wrote: > Why is updateState() being exposed? I think it should be kept private, given its > updating the internal state of the super class. Something needs to set the host, and I wondered why that was done with updateState instead of the constructor. I think I'll move it to the constructor. https://chromiumcodereview.appspot.com/2211353002/diff/40001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java (right): https://chromiumcodereview.appspot.com/2211353002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java:77: return mSurroundingTextSample.substring(mFocusedWordEnd - mSurroundingTextSampleStart); On 2016/08/22 20:54:17, pedro (no code reviews) wrote: > On 2016/08/17 04:35:22, Donn Denman wrote: > > On 2016/08/16 15:41:49, Theresa Wellington wrote: > > > Should this just be mFocusedWordEnd? > > > > > > e.g. mFocusedWordEnd = 10, mSourrindTextSampleStart = 7, so I get a > substring > > > starting at 3? Wouldn't we just want a substring starting at 10? > > > > Oh, this is confusing -- these offsets are within the original full > surrounding > > text, not within the sample! > > > > Pedro, is there good reason why these offsets shouldn't be relative to the > > sample? > > > > I added a TODO to change this. But that's probably best done in the C++ code > > that returns the offsets. > > > > Does that sound right to both of you? > > Goldmine will send us offsets relative to the whole string, not just the sample. > Also Goldmine, has *inclusive* start offsets and *exclusive* end offsets, > represented by the [start, end) interval, which means: start <= offset < end. > > For consistency reasons, and to avoid confusion, all indexes/intervals are > considered to be relative to the whole string, with *inclusive* start offsets > and *exclusive* end offsets. > > Subtracting mSourrindTextSampleStart from a index means converting it to sample > based, as opposed to global based (based on the whole string). > > Global position (relative to whole string): mFocusedWordEnd > Local position (relative to the sample string): mFocusedWordEnd - > mSurroundingTextSampleStart This issue no longer applies -- not sending the surrounding sample to Java anymore. https://chromiumcodereview.appspot.com/2211353002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java:163: protected void updateState(SearchGestureHost host) { On 2016/08/22 20:54:17, pedro (no code reviews) wrote: > I think this should be kept as private, as stated elsewhere. Done -- removed. https://chromiumcodereview.appspot.com/2211353002/diff/40001/chrome/browser/a... File chrome/browser/android/contextualsearch/search_action.cc (right): https://chromiumcodereview.appspot.com/2211353002/diff/40001/chrome/browser/a... chrome/browser/android/contextualsearch/search_action.cc:185: // TODO(pedrosimonetti): should not include CJK characters! On 2016/08/22 20:54:17, pedro (no code reviews) wrote: > On 2016/08/17 04:35:22, Donn Denman wrote: > > On 2016/08/16 15:41:49, Theresa Wellington wrote: > > > Let's be sure to get some manual test coverage on CJK to make sure we're not > > > breaking anything there. > > > > Pedro, why shouldn't CJK characters be included here? Maybe you meant some > > other CJK items like punctuation? > > If we include all CJK charactes, as we are doing, then it will select all > characters until it finds a non-character, like space or punctuation. This is a > problem because our resolution engine will "expand" words when resolving, but > not "shrink". This means resolution will be broken for CJK languages. Instead, > the code will select chunks of characters that not necessarily translate to > actual words. > > Example: "San" might be resolved to "San Francisco", but "San Francisco plus > some extra characters" won't be resolved to "San Francisco". Acknowledged.
I think long-term we want to always keep the surrounding text in native (right? Or are there cases where we still need to send it up to Java?) and that we should take a step back and rework the design doc to figure out how gathering & analyzing surrounding text natively interacts with the original refactoring plan. https://codereview.chromium.org/2211353002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java (right): https://codereview.chromium.org/2211353002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java:91: protected void requestSurroundingText() { Under what scenarios do we want the surrounding text to come back up to Java?
Theresa, thanks for the review! Updated by removing the Java accessors to the tapped text in favor of native accessors. I don't think the architecture for the refactoring changes very much when we move the suppression to native code -- the Java code will just ask the native code to decide if it should be suppressed. We should talk f2f about whether it makes sense to land this partial-refactoring or just do the whole thing. https://codereview.chromium.org/2211353002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java (right): https://codereview.chromium.org/2211353002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java:91: protected void requestSurroundingText() { On 2016/08/26 16:39:07, Theresa Wellington wrote: > Under what scenarios do we want the surrounding text to come back up to Java? That's a good question -- we don't yet have usage for the tapped text. I was thinking we'd do that by extending the current Java suppression but now I think we should just do it all native. Removed these Java accessors to the tapped text.
Theresa and Pedro, I think this is ready for another serious look. This version gathers the surrounding text but does not send to java, just notifies the java code, and provides native access to the tapped text. I think this is all we'll need in order to grab that tapped word to log as a signal, etc. PTAL. Thanks!
lgtm https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java (right): https://codereview.chromium.org/2211353002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:131: if (mHasIdentifiedUnhandledTap) createSearchAction(); On 2016/08/23 23:21:46, Donn Denman wrote: > On 2016/08/22 20:54:17, pedro (no code reviews) wrote: > > Not sure this is the right place to call createAction(). > > > > An action should be created when *any* search gesture is detected. > > > > Similarly, an existing action should be destroyed when *any* other search > > gesture is detected, when the existing action is dismissed by a dismissal > > gesture, or when the Panel gets closed. > > > > Now, we are creating a search action when the tap gesture is detected, but we > > are not creating a new gesture when the long-press gesture is detected. > > > > Also, it seems we're not destroying the actions in all cases we should, like > > when a new gesture is detected, for example. > > In this initial integration of the refactoring we're only using the SearchAction > to get the tapped-text for searchyness evaluation, not for all searches, so > that's why we only make one on Tap. > > I'll make sure that any previous one is destroyed in createSearchAction(). > Thanks for catching that! I think it should also be destroyed when a long press is detected, even if long press gesture is not using the SearchAction. https://codereview.chromium.org/2211353002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java (right): https://codereview.chromium.org/2211353002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:450: void createSearchAction() { The SearchAction should also be destroyed when a long press gesture is detected, even if it's not using the SearchAction class. Also, it should be destroyed when a invalid tap/gesture is detected, I would think. https://codereview.chromium.org/2211353002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java (right): https://codereview.chromium.org/2211353002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java:83: protected boolean shouldSuppressAction() { Is this being used? This is where the tap suppression was supposed to be implemented. I suppose the suppression is not implemented yet. https://codereview.chromium.org/2211353002/diff/120001/chrome/browser/android... File chrome/browser/android/contextualsearch/search_action.cc (right): https://codereview.chromium.org/2211353002/diff/120001/chrome/browser/android... chrome/browser/android/contextualsearch/search_action.cc:97: std::pair<int, int> SearchAction::FindFocusedWord( Nit: Why not merge this method with the one with same name above? https://codereview.chromium.org/2211353002/diff/120001/chrome/browser/android... chrome/browser/android/contextualsearch/search_action.cc:182: // TODO(pedrosimonetti): should not include CJK characters! Take over this TODO? This needs to be addressed or it will break resolution on CJK languages.
https://codereview.chromium.org/2211353002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java (right): https://codereview.chromium.org/2211353002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:409: * gathering surrounding text if needed, etc). nit: ..., etc) but before a selection has been established? https://codereview.chromium.org/2211353002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:451: if (mSearchAction != null) destroySearchAction(); nit: destroySearchAction() checks if mSearchAction == null, so we don't need to check here https://codereview.chromium.org/2211353002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java (right): https://codereview.chromium.org/2211353002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java:32: * gathering the text surrounding a Tap gesture in order to determine whether a search should nit: ... text on a Tap.... ..... determine whether the Tap should be suppressed or a search should be done.... https://codereview.chromium.org/2211353002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java:83: protected boolean shouldSuppressAction() { I think ultimately we want this to come from native, but for now, would it make sense to try to move the TapSuppressionHeuristics checks ResolvedSearchaction#shouldSuppressAction()? https://codereview.chromium.org/2211353002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java:109: nativeRequestSurroundingText(mNativePointer, webContents); Is there a cost associated with gathering the surrounding text even if we're not sending it back up to Java? https://codereview.chromium.org/2211353002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java:116: protected void onSurroundingTextResponse() { Should this be onSurroundingTextReady() since it's not sending the surroundings up anymore? https://codereview.chromium.org/2211353002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java:117: long duration = (System.nanoTime() - mRequestSurroundingTextStartTime) / 1000000; Findbugs will probably complain about this unused variable. https://codereview.chromium.org/2211353002/diff/120001/chrome/browser/android... File chrome/browser/android/contextualsearch/search_action.cc (right): https://codereview.chromium.org/2211353002/diff/120001/chrome/browser/android... chrome/browser/android/contextualsearch/search_action.cc:25: const int kSurroundingTextSize = 1536; nit: where did 1536 come from?
Thanks for the reviews! Theresa, PTAL. https://chromiumcodereview.appspot.com/2211353002/diff/120001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java (right): https://chromiumcodereview.appspot.com/2211353002/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:409: * gathering surrounding text if needed, etc). On 2016/09/01 16:49:36, Theresa Wellington wrote: > nit: ..., etc) but before a selection has been established? Done. Now reads: "This should be called when the associated {@code SearchAction} has built its context (by gathering surrounding text if needed, etc) but before showing any UX." https://chromiumcodereview.appspot.com/2211353002/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:450: void createSearchAction() { On 2016/08/31 22:38:22, pedro (no code reviews) wrote: > The SearchAction should also be destroyed when a long press gesture is detected, > even if it's not using the SearchAction class. > > Also, it should be destroyed when a invalid tap/gesture is detected, I would > think. Done: Added a destroySearchAction to handleSelectionChanged so anytime we get a new selection we'll destroy the search action (for the previous selection). https://chromiumcodereview.appspot.com/2211353002/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:451: if (mSearchAction != null) destroySearchAction(); On 2016/09/01 16:49:36, Theresa Wellington wrote: > nit: destroySearchAction() checks if mSearchAction == null, so we don't need to > check here Done. https://chromiumcodereview.appspot.com/2211353002/diff/120001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java (right): https://chromiumcodereview.appspot.com/2211353002/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java:32: * gathering the text surrounding a Tap gesture in order to determine whether a search should On 2016/09/01 16:49:36, Theresa Wellington wrote: > nit: ... text on a Tap.... > ..... determine whether the Tap should be suppressed or a search should be > done.... Done. https://chromiumcodereview.appspot.com/2211353002/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java:83: protected boolean shouldSuppressAction() { On 2016/09/01 16:49:36, Theresa Wellington wrote: > I think ultimately we want this to come from native, but for now, would it make > sense to try to move the TapSuppressionHeuristics checks > ResolvedSearchaction#shouldSuppressAction()? I'd rather wait until we take the next step in integration of the refactoring, or moving tap suppression to native. Thanks for asking, I had to think pretty hard about this, and that's almost always a good thing! https://chromiumcodereview.appspot.com/2211353002/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java:83: protected boolean shouldSuppressAction() { On 2016/08/31 22:38:22, pedro (no code reviews) wrote: > Is this being used? This is where the tap suppression was supposed to be > implemented. I suppose the suppression is not implemented yet. Added a TODO to integrate with the native tap suppression. https://chromiumcodereview.appspot.com/2211353002/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java:109: nativeRequestSurroundingText(mNativePointer, webContents); On 2016/09/01 16:49:36, Theresa Wellington wrote: > Is there a cost associated with gathering the surrounding text even if we're not > sending it back up to Java? I'm not concerned about this. Looks like the cost is pretty small since we just get back a const& pointer to text so no copy is made. When we implement the searchiness metric we'll always examine that text in native, so it will be useful. Happens only on a tap gesture, so not TOO often. https://chromiumcodereview.appspot.com/2211353002/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java:116: protected void onSurroundingTextResponse() { On 2016/09/01 16:49:37, Theresa Wellington wrote: > Should this be onSurroundingTextReady() since it's not sending the surroundings > up anymore? Done. https://chromiumcodereview.appspot.com/2211353002/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java:117: long duration = (System.nanoTime() - mRequestSurroundingTextStartTime) / 1000000; On 2016/09/01 16:49:37, Theresa Wellington wrote: > Findbugs will probably complain about this unused variable. OK, removed this code. https://chromiumcodereview.appspot.com/2211353002/diff/120001/chrome/browser/... File chrome/browser/android/contextualsearch/search_action.cc (right): https://chromiumcodereview.appspot.com/2211353002/diff/120001/chrome/browser/... chrome/browser/android/contextualsearch/search_action.cc:25: const int kSurroundingTextSize = 1536; On 2016/09/01 16:49:37, Theresa Wellington wrote: > nit: where did 1536 come from? Done: added the comment "Context length is set to 1.5K characters for historical reasons. This provides the largest context that would fit in a GET request." However this raises a reasonable question -- why gather so much text? We are only using this for Tap suppression, so that larger amount of context may not be helpful. We've basically just copied what we do for the resolve, but that might not be best for suppression. However, I think it's possible that the amount of text before or after the selection could be a useful signal, so I think we should just leave it as-is. https://chromiumcodereview.appspot.com/2211353002/diff/120001/chrome/browser/... chrome/browser/android/contextualsearch/search_action.cc:97: std::pair<int, int> SearchAction::FindFocusedWord( On 2016/08/31 22:38:22, pedro (no code reviews) wrote: > Nit: Why not merge this method with the one with same name above? Done. It was tricky to figure out how to test non-static methods of this class since it's linked to a Java class. Ended up adding a private default constructor for tests, and checking in the destructor if there's any linked Java object. Seems like a good approach. https://chromiumcodereview.appspot.com/2211353002/diff/120001/chrome/browser/... chrome/browser/android/contextualsearch/search_action.cc:182: // TODO(pedrosimonetti): should not include CJK characters! On 2016/08/31 22:38:22, pedro (no code reviews) wrote: > Take over this TODO? This needs to be addressed or it will break resolution on > CJK languages. Done. Note: With the current implementation surroundings for the resolution are still handled by the CS delegate, so resolution should be OK for now. It will break the suppression logic however.
lgtm % 1 request for a TODO https://codereview.chromium.org/2211353002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java (right): https://codereview.chromium.org/2211353002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java:109: nativeRequestSurroundingText(mNativePointer, webContents); On 2016/09/02 16:40:43, Donn Denman wrote: > On 2016/09/01 16:49:36, Theresa Wellington wrote: > > Is there a cost associated with gathering the surrounding text even if we're > not > > sending it back up to Java? > > I'm not concerned about this. Looks like the cost is pretty small since we just > get back a const& pointer to text so no copy is made. When we implement the > searchiness metric we'll always examine that text in native, so it will be > useful. Happens only on a tap gesture, so not TOO often. I was thinking more about the blink operation that gathers the surrounding text in the first place, rather than the storage required. Is there a way to cache the surrounding text in native so that we don't gather it twice? Probably not blocking for this CL but a TODO would be good. https://codereview.chromium.org/2211353002/diff/140001/chrome/browser/android... File chrome/browser/android/contextualsearch/search_action.cc (right): https://codereview.chromium.org/2211353002/diff/140001/chrome/browser/android... chrome/browser/android/contextualsearch/search_action.cc:26: // This provides the largest context that would fit in a GET request along with nit: s/would/will
donnd@chromium.org changed reviewers: + tedchoc@chromium.org
Ted PTAL at chrome_jni_registrar.cc for owners approval. Theresa and Pedro, thanks for the review! https://chromiumcodereview.appspot.com/2211353002/diff/120001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java (right): https://chromiumcodereview.appspot.com/2211353002/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java:109: nativeRequestSurroundingText(mNativePointer, webContents); On 2016/09/02 17:04:07, Theresa Wellington wrote: > On 2016/09/02 16:40:43, Donn Denman wrote: > > On 2016/09/01 16:49:36, Theresa Wellington wrote: > > > Is there a cost associated with gathering the surrounding text even if we're > > not > > > sending it back up to Java? > > > > I'm not concerned about this. Looks like the cost is pretty small since we > just > > get back a const& pointer to text so no copy is made. When we implement the > > searchiness metric we'll always examine that text in native, so it will be > > useful. Happens only on a tap gesture, so not TOO often. > > I was thinking more about the blink operation that gathers the surrounding text > in the first place, rather than the storage required. Is there a way to cache > the surrounding text in native so that we don't gather it twice? Probably not > blocking for this CL but a TODO would be good. Done. It's actually stored in the native context already. I think we just need to complete the refactoring to prevent making the additional request.... although the selection does change when we call SelectWordAroundCaret so we'll need to be careful if we decide to reuse it. https://chromiumcodereview.appspot.com/2211353002/diff/140001/chrome/browser/... File chrome/browser/android/contextualsearch/search_action.cc (right): https://chromiumcodereview.appspot.com/2211353002/diff/140001/chrome/browser/... chrome/browser/android/contextualsearch/search_action.cc:26: // This provides the largest context that would fit in a GET request along with On 2016/09/02 17:04:07, Theresa Wellington wrote: > nit: s/would/will Done slightly differently -- now phrased fully in the past tense "This provided ... that would fit..."
On 2016/09/02 20:48:35, Donn Denman wrote: > Ted PTAL at chrome_jni_registrar.cc for owners approval. > > Theresa and Pedro, thanks for the review! > > https://chromiumcodereview.appspot.com/2211353002/diff/120001/chrome/android/... > File > chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java > (right): > > https://chromiumcodereview.appspot.com/2211353002/diff/120001/chrome/android/... > chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/action/SearchAction.java:109: > nativeRequestSurroundingText(mNativePointer, webContents); > On 2016/09/02 17:04:07, Theresa Wellington wrote: > > On 2016/09/02 16:40:43, Donn Denman wrote: > > > On 2016/09/01 16:49:36, Theresa Wellington wrote: > > > > Is there a cost associated with gathering the surrounding text even if > we're > > > not > > > > sending it back up to Java? > > > > > > I'm not concerned about this. Looks like the cost is pretty small since we > > just > > > get back a const& pointer to text so no copy is made. When we implement the > > > searchiness metric we'll always examine that text in native, so it will be > > > useful. Happens only on a tap gesture, so not TOO often. > > > > I was thinking more about the blink operation that gathers the surrounding > text > > in the first place, rather than the storage required. Is there a way to cache > > the surrounding text in native so that we don't gather it twice? Probably not > > blocking for this CL but a TODO would be good. > > Done. It's actually stored in the native context already. I think we just need > to complete the refactoring to prevent making the additional request.... > although the selection does change when we call SelectWordAroundCaret so we'll > need to be careful if we decide to reuse it. > > https://chromiumcodereview.appspot.com/2211353002/diff/140001/chrome/browser/... > File chrome/browser/android/contextualsearch/search_action.cc (right): > > https://chromiumcodereview.appspot.com/2211353002/diff/140001/chrome/browser/... > chrome/browser/android/contextualsearch/search_action.cc:26: // This provides > the largest context that would fit in a GET request along with > On 2016/09/02 17:04:07, Theresa Wellington wrote: > > nit: s/would/will > > Done slightly differently -- now phrased fully in the past tense "This provided > ... that would fit..." chrome_jni_registrar.cc - lgtm
The CQ bit was checked by donnd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pedrosimonetti@chromium.org, twellington@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2211353002/#ps160001 (title: "Updated a comment and added a TODO.")
The CQ bit was unchecked by donnd@chromium.org
The CQ bit was checked by donnd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, twellington@chromium.org, pedrosimonetti@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2211353002/#ps200001 (title: "Just another rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by donnd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Theresa, I had to make some changes to the Instrumentation tests in order to get this CL to work. You may want to take another look, so please give this another LGTM when you've done as much looking as you care to. The problem was 2 tests failed when they called tapBasePageToClosePanel after a tap to select. The second tap, which was supposed to close the panel, actually selected text and brought up the Bar instead. I'm pretty sure this is due to the taps happening so fast that the delay involved with gathering the surrounding text caused our tap-far-from-previous to fail. Since Pedro had a TODO to get rid of tapBasePageToClosePanel I went that route, and it seems to work, so hopefully we'll get some dividends in more reliable tests, rather than some test regression. I did double-check that we test tap-far-from-previous, that they are unchanged and they still work, so I think this is probably the way to go. Let me know if you have Qs about this!
still lgtm https://chromiumcodereview.appspot.com/2211353002/diff/220001/chrome/android/... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java (right): https://chromiumcodereview.appspot.com/2211353002/diff/220001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:807: private void tapBasePage(float x, float y) { Is this being used anywhere anymore? https://chromiumcodereview.appspot.com/2211353002/diff/220001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:824: * Tap on the peeking Bar to expand the panel, then taps on the base page to close it. this comment needs to be updated.
Thanks Theresa! https://chromiumcodereview.appspot.com/2211353002/diff/220001/chrome/android/... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java (right): https://chromiumcodereview.appspot.com/2211353002/diff/220001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:807: private void tapBasePage(float x, float y) { On 2016/09/12 22:59:19, Theresa Wellington wrote: > Is this being used anywhere anymore? Removed. Also consolidated to methods just below. https://chromiumcodereview.appspot.com/2211353002/diff/220001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:824: * Tap on the peeking Bar to expand the panel, then taps on the base page to close it. On 2016/09/12 22:59:19, Theresa Wellington wrote: > this comment needs to be updated. Removed due to consolidation. Cleaned up a few other similar comments.
The CQ bit was checked by donnd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, pedrosimonetti@chromium.org, twellington@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2211353002/#ps240001 (title: "Removed unused code and consolidated two functions, updated comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [TTS] Gather surrounding text on Tap before any UX. Extract the text tapped on to use as a signal in Tap Suppression. The text is extracted before any UX is displayed in order to allow the tap to be totally ignored when appropriate. Feeding the surrounding text into the logic of TTS will be done separately. This adds several files that are part of the 2016-refactoring. See crbug.com/624609 and go/cs-refactoring-2016. This CL is part of the refactoring-2016 effort, see go/cs-refactoring-2016 for details. BUG=634136, 624609 ========== to ========== [TTS] Gather surrounding text on Tap before any UX. Extract the text tapped on to use as a signal in Tap Suppression. The text is extracted before any UX is displayed in order to allow the tap to be totally ignored when appropriate. Feeding the surrounding text into the logic of TTS will be done separately. This adds several files that are part of the 2016-refactoring. See crbug.com/624609 and go/cs-refactoring-2016. This CL is part of the refactoring-2016 effort, see go/cs-refactoring-2016 for details. BUG=634136, 624609 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== [TTS] Gather surrounding text on Tap before any UX. Extract the text tapped on to use as a signal in Tap Suppression. The text is extracted before any UX is displayed in order to allow the tap to be totally ignored when appropriate. Feeding the surrounding text into the logic of TTS will be done separately. This adds several files that are part of the 2016-refactoring. See crbug.com/624609 and go/cs-refactoring-2016. This CL is part of the refactoring-2016 effort, see go/cs-refactoring-2016 for details. BUG=634136, 624609 ========== to ========== [TTS] Gather surrounding text on Tap before any UX. Extract the text tapped on to use as a signal in Tap Suppression. The text is extracted before any UX is displayed in order to allow the tap to be totally ignored when appropriate. Feeding the surrounding text into the logic of TTS will be done separately. This adds several files that are part of the 2016-refactoring. See crbug.com/624609 and go/cs-refactoring-2016. This CL is part of the refactoring-2016 effort, see go/cs-refactoring-2016 for details. BUG=634136, 624609 Committed: https://crrev.com/3f11e42880b1b7b6608e2d57fa538552318367ae Cr-Commit-Position: refs/heads/master@{#418464} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/3f11e42880b1b7b6608e2d57fa538552318367ae Cr-Commit-Position: refs/heads/master@{#418464}
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/2348443002/ by jbroman@chromium.org. The reason for reverting is: Suspected of causing try flakes in ContextualSearchManagerTest#testPromoTapCount: https://bugs.chromium.org/p/chromium/issues/detail?id=647210. |
