Adds selection expansion support for Contextual Search.

Adds selection expansion support for Contextual Search.

This CL adds functionality to expand the existing text selection
when the contextual search server returns what the user is most
likely to search for.

BUG=396738
Committed: https://crrev.com/ab031902f8b4b8a0fb9892435f1ccdbad30e89cf
Cr-Commit-Position: refs/heads/master@{#338374}

[aurimas (slooooooooow), 2015-06-25 00:07:22]

aurimas@chromium.org changed reviewers:
+ dtrainor@chromium.org, pedrosimonetti@chromium.org

[aurimas (slooooooooow), 2015-06-25 00:07:23]

dtrainor: content/public/android and chrome/browser/android
pedrosimonetti: general review

[David Trainor- moved to gerrit, 2015-06-25 00:34:55]

lgtm with nits

https://chromiumcodereview.appspot.com/1205033005/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java
(right):

https://chromiumcodereview.appspot.com/1205033005/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:260:
void adjustSelection(int selectionStartAdjust, int selectionEndAdjust) {
Eh might as well flesh out this javadoc with the same strings as above.  Should
we also make sure selectionStartAdjust and selectionEndAdjust aren't both 0 (I'm
assuming they're both offsets from the current selection points)?  I'm imagining
that doing weird things with the early out above if the user tries to refine the
selection.

https://chromiumcodereview.appspot.com/1205033005/diff/1/content/renderer/ren...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/1205033005/diff/1/content/renderer/ren...
content/renderer/render_view_impl.cc:1404: length += end_adjust - start_adjust;
Do some checks on this stuff to make sure these values are valid after the
adjustments.  size_t is unsigned right?

[pedro (no code reviews), 2015-06-25 01:30:28]

Overall, this CL looks good to me. I think we can improve the name of methods to
make things clearer though.

We have been always referring to this feature as "selection expansion" so I
believe the API should reflect this terminology. Instead, now the code sometimes
call it "adjust" or "move" selection.

In my opinion, "adjust" it's too broad (adjust can mean several things, like
shrinking), and "move" is not exactly what we're doing with the selection (move
implies the length does not change, which is not the case). For these reasons, I
believe calling it "expand" in all places will make the code easier to
understand and the API easier to remember.

https://codereview.chromium.org/1205033005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
(right):

https://codereview.chromium.org/1205033005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:718:
mSelectionController.adjustSelection(selectionStartAdjust, selectionEndAdjust);
Nit: Call it expandSelection() instead?

https://codereview.chromium.org/1205033005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java
(right):

https://codereview.chromium.org/1205033005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:49:
private boolean mDidAdjustSelection;
Nit: mDidAdjustSelection --> mDidExpandSelection

https://codereview.chromium.org/1205033005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:263:
if (basePageContentView != null) {
Probably it's worth checking that basePageContentView.getWebContents() is also
not null

https://codereview.chromium.org/1205033005/diff/1/chrome/browser/android/cont...
File chrome/browser/android/contextualsearch/contextual_search_delegate.cc
(right):

https://codereview.chromium.org/1205033005/diff/1/chrome/browser/android/cont...
chrome/browser/android/contextualsearch/contextual_search_delegate.cc:432:
base::ListValue* mentions_list;
Does this represent multiple mentions. If so, are we supposed to only look at
the first mention (as this CL is currently doing)?

https://codereview.chromium.org/1205033005/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/1205033005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_impl.h:180: void
MoveSelectionByCharacterOffset(int startAdjust, int endAdjust) override;
nit: MoveSelectionByCharacterOffset() --> ExpandSelectionByCharacterOffset()

[aurimas (slooooooooow), 2015-07-01 00:45:37]

aurimas@chromium.org changed reviewers:
+ palmer@chromium.org, sievers@chromium.org, tedchoc@chromium.org

[aurimas (slooooooooow), 2015-07-01 00:45:38]

+palmer for content/common/view_messages.h
+tedchoc for content/browser/web_contents/* and content/public/android/*
+sievers for content/browser/renderer_host/* content/renderer/*
content/public/browser/*

https://chromiumcodereview.appspot.com/1205033005/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
(right):

https://chromiumcodereview.appspot.com/1205033005/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:718:
mSelectionController.adjustSelection(selectionStartAdjust, selectionEndAdjust);
On 2015/06/25 at 01:30:28, pedrosimonetti wrote:
> Nit: Call it expandSelection() instead?

Done

https://chromiumcodereview.appspot.com/1205033005/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java
(right):

https://chromiumcodereview.appspot.com/1205033005/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:49:
private boolean mDidAdjustSelection;
On 2015/06/25 at 01:30:28, pedrosimonetti wrote:
> Nit: mDidAdjustSelection --> mDidExpandSelection

Done

https://chromiumcodereview.appspot.com/1205033005/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:263:
if (basePageContentView != null) {
On 2015/06/25 at 01:30:28, pedrosimonetti wrote:
> Probably it's worth checking that basePageContentView.getWebContents() is also
not null

Done

[pedro (no code reviews), 2015-07-01 22:18:57]

lgtm

Thanks Aurimas, Contextual Search code looks good to me. Just posted a few nits.

Someone more familiar with the renderer code should review the rest of the code,
since my knowledge of this part of the code is limited and I might me missing
things.

https://codereview.chromium.org/1205033005/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
(right):

https://codereview.chromium.org/1205033005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:643:
*        the search term.
I would add to make things clearer:

This number should be non-negative and represents the number of characters the
selection will be expanded at the beginning of the existing selection.

(or something similar)

https://codereview.chromium.org/1205033005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:645:
*        the search term.
Same thing as above, but replacing "beginning" with "end".

https://codereview.chromium.org/1205033005/diff/60001/chrome/browser/android/...
File chrome/browser/android/contextualsearch/contextual_search_delegate.cc
(right):

https://codereview.chromium.org/1205033005/diff/60001/chrome/browser/android/...
chrome/browser/android/contextualsearch/contextual_search_delegate.cc:163:
end_adjust = std::max(0, end_adjust);
Nit: Put a TODO here to investigate why sometimes the selection is offset by 1?
Also, put a link to the bug.

https://codereview.chromium.org/1205033005/diff/60001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1205033005/diff/60001/content/renderer/render...
content/renderer/render_view_impl.cc:1409:
webview()->focusedFrame()->toWebLocalFrame();
Please confirm with someone familiar with the WebView if we really need
toWebLocalFrame(), or if we could use instead just focusedFrame() like we do
above for OnSelectWordAroundCaret(). Mounir mentioned that some things are
moving the the local frame, but I don't really understand the difference of the
two.

[aurimas (slooooooooow), 2015-07-01 23:05:45]

https://chromiumcodereview.appspot.com/1205033005/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
(right):

https://chromiumcodereview.appspot.com/1205033005/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:643:
*        the search term.
On 2015/07/01 at 22:18:56, pedrosimonetti wrote:
> I would add to make things clearer:
> 
> This number should be non-negative and represents the number of characters the
selection will be expanded at the beginning of the existing selection.
> 
> (or something similar)

Done

https://chromiumcodereview.appspot.com/1205033005/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:645:
*        the search term.
On 2015/07/01 at 22:18:56, pedrosimonetti wrote:
> Same thing as above, but replacing "beginning" with "end".

Done

https://chromiumcodereview.appspot.com/1205033005/diff/60001/chrome/browser/a...
File chrome/browser/android/contextualsearch/contextual_search_delegate.cc
(right):

https://chromiumcodereview.appspot.com/1205033005/diff/60001/chrome/browser/a...
chrome/browser/android/contextualsearch/contextual_search_delegate.cc:163:
end_adjust = std::max(0, end_adjust);
On 2015/07/01 at 22:18:57, pedrosimonetti wrote:
> Nit: Put a TODO here to investigate why sometimes the selection is offset by
1? Also, put a link to the bug.

Done

https://chromiumcodereview.appspot.com/1205033005/diff/60001/content/renderer...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/1205033005/diff/60001/content/renderer...
content/renderer/render_view_impl.cc:1409:
webview()->focusedFrame()->toWebLocalFrame();
On 2015/07/01 at 22:18:57, pedrosimonetti wrote:
> Please confirm with someone familiar with the WebView if we really need
toWebLocalFrame(), or if we could use instead just focusedFrame() like we do
above for OnSelectWordAroundCaret(). Mounir mentioned that some things are
moving the the local frame, but I don't really understand the difference of the
two.

Do you have suggestions about who to ask to review this?

[pedro (no code reviews), 2015-07-02 00:09:57]

https://codereview.chromium.org/1205033005/diff/60001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1205033005/diff/60001/content/renderer/render...
content/renderer/render_view_impl.cc:1409:
webview()->focusedFrame()->toWebLocalFrame();
On 2015/07/01 23:05:44, aurimas wrote:
> On 2015/07/01 at 22:18:57, pedrosimonetti wrote:
> > Please confirm with someone familiar with the WebView if we really need
> toWebLocalFrame(), or if we could use instead just focusedFrame() like we do
> above for OnSelectWordAroundCaret(). Mounir mentioned that some things are
> moving the the local frame, but I don't really understand the difference of
the
> two.
> 
> Do you have suggestions about who to ask to review this?

I would say yoichio@ since he is the one who suggested using this approach:
https://code.google.com/p/chromium/issues/detail?id=396738#c20

[aurimas (slooooooooow), 2015-07-02 00:20:36]

aurimas@chromium.org changed reviewers:
+ yoichio@chromium.org

[aurimas (slooooooooow), 2015-07-02 00:20:36]

+yoichio: please take a look at content/render/render_view_impl.*

[yoichio, 2015-07-02 04:19:57]

lgtm

[aurimas (slooooooooow), 2015-07-06 18:15:16]

Post 4th of July ping:

+palmer for content/common/view_messages.h
+tedchoc for content/browser/web_contents/* and content/public/android/*
+sievers for content/browser/renderer_host/* content/renderer/*
content/public/browser/*

[Ted C, 2015-07-06 20:56:46]

On 2015/07/06 18:15:16, aurimas wrote:
> Post 4th of July ping:
> 
> +palmer for content/common/view_messages.h
> +tedchoc for content/browser/web_contents/* and content/public/android/*
> +sievers for content/browser/renderer_host/* content/renderer/*
> content/public/browser/*

content/browser/web_contents && content/public/android - lgtm

[aurimas (slooooooooow), 2015-07-06 21:00:22]

aurimas@chromium.org changed reviewers:
+ aelias@chromium.org, creis@chromium.org

[aurimas (slooooooooow), 2015-07-06 21:00:22]

Since sievers@ is OOO:

+aelias for content/browser/renderer_host/*
+creis for content/renderer/*

[aelias_OOO_until_Jul13, 2015-07-06 21:08:17]

content/browser/renderer_host/ lgtm

[Charlie Reis, 2015-07-06 21:30:41]

https://codereview.chromium.org/1205033005/diff/80001/content/public/browser/...
File content/public/browser/render_view_host.h (right):

https://codereview.chromium.org/1205033005/diff/80001/content/public/browser/...
content/public/browser/render_view_host.h:55: // DEPRECATED: RenderViewHost is
being removed as part of the SiteIsolation
This class is deprecated because it doesn't support out-of-process iframes.  Is
there a way to move this code to RenderFrameHost?  Should all the selection code
be moved there to support out-of-process iframes?

https://codereview.chromium.org/1205033005/diff/80001/content/public/browser/...
content/public/browser/render_view_host.h:203: int end_adjust) = 0;
Is it possible for this to cross a frame boundary?  If so, what happens if the
frame lives in a different process?

https://codereview.chromium.org/1205033005/diff/80001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/1205033005/diff/80001/content/renderer/render...
content/renderer/render_view_impl.h:147: // DEPRECATED: RenderViewImpl is being
removed as part of the SiteIsolation
RenderView is also deprecated.

[Charlie Reis, 2015-07-06 21:32:15]

[+site-isolation-reviews, in case we need to track changes for OOPIF support for
text selection.]

[aurimas (slooooooooow), 2015-07-07 00:49:12]

creis: please take another look. I moved over to using render_frame_*, but I am
not sure that I am getting to the RenderFrameHost correctly (via WebContents).

[Charlie Reis, 2015-07-07 17:02:28]

Thanks!  That's much better.

content/ LGTM with nits, though it would be nice to have a test for the actual
expansion of the selection if that's doable.  (I'm not sure how the input event
IPCs are tested in general.)

https://codereview.chromium.org/1205033005/diff/100001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1205033005/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2060: RenderFrameHost*
focused_frame = GetFocusedFrame();
Yes, operating on the focused frame sounds right to me.

https://codereview.chromium.org/1205033005/diff/100001/content/public/browser...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1205033005/diff/100001/content/public/browser...
content/public/browser/web_contents.h:420: // Notify the render view host to
adjust the expand selection by the given
nit: Expand the selection in the focused frame by the given amounts.

https://codereview.chromium.org/1205033005/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1205033005/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1340: 
nit: No blank line.

[aurimas (slooooooooow), 2015-07-07 18:18:05]

https://chromiumcodereview.appspot.com/1205033005/diff/100001/content/public/...
File content/public/browser/web_contents.h (right):

https://chromiumcodereview.appspot.com/1205033005/diff/100001/content/public/...
content/public/browser/web_contents.h:420: // Notify the render view host to
adjust the expand selection by the given
On 2015/07/07 at 17:02:27, Charlie Reis wrote:
> nit: Expand the selection in the focused frame by the given amounts.

Done

https://chromiumcodereview.appspot.com/1205033005/diff/100001/content/rendere...
File content/renderer/render_frame_impl.cc (right):

https://chromiumcodereview.appspot.com/1205033005/diff/100001/content/rendere...
content/renderer/render_frame_impl.cc:1340: 
On 2015/07/07 at 17:02:27, Charlie Reis wrote:
> nit: No blank line.

Done

[aurimas (slooooooooow), 2015-07-07 18:19:09]

tedchoc: could you take another pass at content/browser/web_contents as I
changed the code to use RenderFrame* instead of RenderView

[palmer, 2015-07-07 18:40:00]

I'm not fully clear on the intended semantics, as you can see. Apologies if it's
obvious and I'm missing it. That's entirely possible. :)

See also
https://www.chromium.org/Home/chromium-security/education/security-tips-for-i....
if you haven't already.

https://codereview.chromium.org/1205033005/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java
(right):

https://codereview.chromium.org/1205033005/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:297:
// TODO(donnd): add code to verify that the selection is still valid before
changing it!
Add a bug link to this comment.

https://codereview.chromium.org/1205033005/diff/120001/chrome/browser/android...
File chrome/browser/android/contextualsearch/contextual_search_delegate.cc
(right):

https://codereview.chromium.org/1205033005/diff/120001/chrome/browser/android...
chrome/browser/android/contextualsearch/contextual_search_delegate.cc:161:
end_adjust = mention_end - context_->end_offset;
I'm finding the implications of the implicit type conversions here to be
non-obvious. I'm wondering if perhaps |start_adjust| and |end_adjust| should
also be declared size_t, and then enforce sane ranges here (e.g. [0, 2**20) ).
Does that make sense?

https://codereview.chromium.org/1205033005/diff/120001/chrome/browser/android...
chrome/browser/android/contextualsearch/contextual_search_delegate.cc:478:
*startResult = int_value;
More strange type-cast semantics. It's OK if the ultimate consumer of these
values protects itself from strange results (as described above). But it looks
odd even so. Maybe a comment to the effect that the (specifically named)
ultimate consumer does protect itself?

https://codereview.chromium.org/1205033005/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_contents_android.h (right):

https://codereview.chromium.org/1205033005/diff/120001/content/browser/web_co...
content/browser/web_contents/web_contents_android.h:82: jint start_adjust,
(Be aware that jsize exists and may be more appropriate if you standardize these
values as size_t in C++.)

https://codereview.chromium.org/1205033005/diff/120001/content/public/android...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
(right):

https://codereview.chromium.org/1205033005/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:183:
* A negative amount moves the selection towards the beginning of the document, a
positive
I may be missing a detail, but elsewhere you state
(ContextualSearchManager.java) and enforce (contextual_search_delegate.cc) that
these values are >= 0. Or, are these different values? The names are similar so
I'm a bit confused.

[aurimas (slooooooooow), 2015-07-09 00:20:50]

palmer@ could you please take another pass?

https://chromiumcodereview.appspot.com/1205033005/diff/120001/chrome/browser/...
File chrome/browser/android/contextualsearch/contextual_search_delegate.cc
(right):

https://chromiumcodereview.appspot.com/1205033005/diff/120001/chrome/browser/...
chrome/browser/android/contextualsearch/contextual_search_delegate.cc:161:
end_adjust = mention_end - context_->end_offset;
On 2015/07/07 at 18:39:59, palmer wrote:
> I'm finding the implications of the implicit type conversions here to be
non-obvious. I'm wondering if perhaps |start_adjust| and |end_adjust| should
also be declared size_t, and then enforce sane ranges here (e.g. [0, 2**20) ).
Does that make sense?

I added limits to make sure selection is non-zero and it is under 100 characters
as such selections would dismiss contextual search bar anyway.

https://chromiumcodereview.appspot.com/1205033005/diff/120001/chrome/browser/...
chrome/browser/android/contextualsearch/contextual_search_delegate.cc:478:
*startResult = int_value;
On 2015/07/07 at 18:39:59, palmer wrote:
> More strange type-cast semantics. It's OK if the ultimate consumer of these
values protects itself from strange results (as described above). But it looks
odd even so. Maybe a comment to the effect that the (specifically named)
ultimate consumer does protect itself?

It now makes it 0 or larger.

https://chromiumcodereview.appspot.com/1205033005/diff/120001/content/public/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
(right):

https://chromiumcodereview.appspot.com/1205033005/diff/120001/content/public/...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:183:
* A negative amount moves the selection towards the beginning of the document, a
positive
On 2015/07/07 at 18:40:00, palmer wrote:
> I may be missing a detail, but elsewhere you state
(ContextualSearchManager.java) and enforce (contextual_search_delegate.cc) that
these values are >= 0. Or, are these different values? The names are similar so
I'm a bit confused.

Sorry for the confusion. This patch went through changes and it did not get
fully updated everywhere. It went through allowing contracting the selection ->
only expansion -> and now in my latest CL it allows to contract again. Final
state is to allow selection contraction as it turns out in Korean Chrome tends
to select several words due to space character usage (it combines "country is"
as one selection) on the client and server can do a better job.

[palmer, 2015-07-09 21:15:17]

https://codereview.chromium.org/1205033005/diff/160001/chrome/browser/android...
File chrome/browser/android/contextualsearch/contextual_search_delegate.cc
(right):

https://codereview.chromium.org/1205033005/diff/160001/chrome/browser/android...
chrome/browser/android/contextualsearch/contextual_search_delegate.cc:169:
end_adjust = mention_end - context_->end_offset;
Do you also need to check if |start_adjust| and |end_adjust| are >
kContextualSearchMaxSelection? We could end up executing this block if
|mention_start| and |mention_end| are huge but (mention_end - mention_start) is
still <= kContextualSearchMaxSelection.

https://codereview.chromium.org/1205033005/diff/160001/chrome/browser/android...
File
chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc
(right):

https://codereview.chromium.org/1205033005/diff/160001/chrome/browser/android...
chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc:288:
TEST_F(ContextualSearchDelegateTest, ExtractMentionsStartEnd) {
It'd be a good idea to add some negative tests with crazy start and end values.

[aurimas (slooooooooow), 2015-07-10 01:46:49]

https://codereview.chromium.org/1205033005/diff/160001/chrome/browser/android...
File chrome/browser/android/contextualsearch/contextual_search_delegate.cc
(right):

https://codereview.chromium.org/1205033005/diff/160001/chrome/browser/android...
chrome/browser/android/contextualsearch/contextual_search_delegate.cc:169:
end_adjust = mention_end - context_->end_offset;
On 2015/07/09 at 21:15:16, palmer wrote:
> Do you also need to check if |start_adjust| and |end_adjust| are >
kContextualSearchMaxSelection? We could end up executing this block if
|mention_start| and |mention_end| are huge but (mention_end - mention_start) is
still <= kContextualSearchMaxSelection.

Great catch. We discussed it and we decided that there should be at least one
character overlap between the old and the new selection.

https://codereview.chromium.org/1205033005/diff/160001/chrome/browser/android...
File
chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc
(right):

https://codereview.chromium.org/1205033005/diff/160001/chrome/browser/android...
chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc:288:
TEST_F(ContextualSearchDelegateTest, ExtractMentionsStartEnd) {
On 2015/07/09 at 21:15:17, palmer wrote:
> It'd be a good idea to add some negative tests with crazy start and end
values.

Added a bunch of tests.

[Donn Denman, 2015-07-10 17:47:02]

donnd@chromium.org changed reviewers:
+ donnd@chromium.org

[Donn Denman, 2015-07-10 17:47:03]

LGTM -- just a nit and a suggestion.  Thanks!

https://chromiumcodereview.appspot.com/1205033005/diff/180001/chrome/browser/...
File
chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc
(right):

https://chromiumcodereview.appspot.com/1205033005/diff/180001/chrome/browser/...
chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc:278:
CreateSearchContextAndRequestSearchTerm("Barack", surrounding, 0, 6);
Looks like this line is indented too much on all these tests?

https://chromiumcodereview.appspot.com/1205033005/diff/180001/chrome/browser/...
chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc:281:
fetcher()->SetResponseString(
If all the response strings for these tests are the same except for mentions,
maybe make a helper to just pass in the different mentions?

[palmer, 2015-07-10 17:53:38]

LGTM. Maybe add some weirder test cases.

https://codereview.chromium.org/1205033005/diff/180001/chrome/browser/android...
File
chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc
(right):

https://codereview.chromium.org/1205033005/diff/180001/chrome/browser/android...
chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc:278:
CreateSearchContextAndRequestSearchTerm("Barack", surrounding, 0, 6);
Nit: Indentation

https://codereview.chromium.org/1205033005/diff/180001/chrome/browser/android...
chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc:366:
"Barack Obama just", surrounding, 0, 17);
What happens if you do crazy things like 

    CreateSearchContextAndRequestSearchTerm("Barack Obama just", surrounding,
0xffffffff, 0xffffffff - 99);

and so on?

[aurimas (slooooooooow), 2015-07-10 19:06:00]

https://chromiumcodereview.appspot.com/1205033005/diff/180001/chrome/browser/...
File
chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc
(right):

https://chromiumcodereview.appspot.com/1205033005/diff/180001/chrome/browser/...
chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc:278:
CreateSearchContextAndRequestSearchTerm("Barack", surrounding, 0, 6);
On 2015/07/10 at 17:53:38, palmer wrote:
> Nit: Indentation

Done

https://chromiumcodereview.appspot.com/1205033005/diff/180001/chrome/browser/...
chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc:281:
fetcher()->SetResponseString(
On 2015/07/10 at 17:47:03, Donn Denman wrote:
> If all the response strings for these tests are the same except for mentions,
maybe make a helper to just pass in the different mentions?

Done

https://chromiumcodereview.appspot.com/1205033005/diff/180001/chrome/browser/...
chrome/browser/android/contextualsearch/contextual_search_delegate_unittest.cc:366:
"Barack Obama just", surrounding, 0, 17);
On 2015/07/10 at 17:53:38, palmer wrote:
> What happens if you do crazy things like 
> 
>     CreateSearchContextAndRequestSearchTerm("Barack Obama just", surrounding,
0xffffffff, 0xffffffff - 99);
> 
> and so on?

Added a few more cases.

[aurimas (slooooooooow), 2015-07-10 19:06:05]

The CQ bit was checked by aurimas@chromium.org to run a CQ dry run

[aurimas (slooooooooow), 2015-07-10 19:06:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, pedrosimonetti@chromium.org, yoichio@chromium.org,
aelias@chromium.org, tedchoc@chromium.org, creis@chromium.org,
donnd@chromium.org, palmer@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1205033005/#ps200001 (title: "Add more
tests")

[commit-bot: I haz the power, 2015-07-10 19:07:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1205033005/200001

[commit-bot: I haz the power, 2015-07-10 19:26:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-10 19:26:55]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)

(exceeded global retry quota)

[aurimas (slooooooooow), 2015-07-10 20:39:36]

The CQ bit was checked by aurimas@chromium.org to run a CQ dry run

[aurimas (slooooooooow), 2015-07-10 20:39:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
yoichio@chromium.org, donnd@chromium.org, aelias@chromium.org,
dtrainor@chromium.org, pedrosimonetti@chromium.org, tedchoc@chromium.org,
palmer@chromium.org, creis@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1205033005/#ps220001 (title: "Fixed java
tests")

[commit-bot: I haz the power, 2015-07-10 20:40:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1205033005/220001

[commit-bot: I haz the power, 2015-07-10 21:46:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-10 21:46:43]

Dry run: This issue passed the CQ dry run.

[aurimas (slooooooooow), 2015-07-10 21:50:32]

The CQ bit was checked by aurimas@chromium.org

[commit-bot: I haz the power, 2015-07-10 21:51:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1205033005/220001

[commit-bot: I haz the power, 2015-07-10 21:57:54]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-07-10 21:58:49]

Patchset 12 (id:??) landed as
https://crrev.com/ab031902f8b4b8a0fb9892435f1ccdbad30e89cf
Cr-Commit-Position: refs/heads/master@{#338374}