Remove ContentViewCore::RequestTextSurroundingSelection

Remove ContentViewCore::RequestTextSurroundingSelection

This change takes care of scaling down the scope of CVC by
removing RequestTextSurroundingSelection() method. Logic has been
moved to RenderFrameHost. Additionally added the support of responding
with empty text when a new request comes during processing of a pending
request.

BUG=626770
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/deb84849f66aa9db12a4c4bd8eb0bb3e18fd441d
Cr-Commit-Position: refs/heads/master@{#406812}

[AKV, 2016-07-16 14:56:01]

Description was changed from

==========
Remove ContentViewCore::RequestTextSurroundingSelection

This change takes care of scaling down the scope of CVC by
removing RequestTextSurroundingSelection() method. Logic has been
moved to RenderFrameHost. Additionally added the support of responding
with empty text when a new request comes during processing of a pending
request.

BUG=626770
==========

to

==========
Remove ContentViewCore::RequestTextSurroundingSelection

This change takes care of scaling down the scope of CVC by
removing RequestTextSurroundingSelection() method. Logic has been
moved to RenderFrameHost. Additionally added the support of responding
with empty text when a new request comes during processing of a pending
request.

BUG=626770
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[AKV, 2016-07-16 15:06:28]

ajith.v@chromium.org changed reviewers:
+ sievers@chromium.org

[AKV, 2016-07-16 15:06:29]

PTAL!

[no sievers, 2016-07-18 22:41:18]

thanks! just one nit.

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

https://codereview.chromium.org/2157793002/diff/1/chrome/browser/android/cont...
chrome/browser/android/contextualsearch/contextual_search_delegate.cc:247: if
(!focused_frame || focused_frame->HasTextSurroundingSelectionCallback()) {
Can you remove HasTextSurroundingSelectionCallback() from RenderFrameHost's
interface and just handle this internally there?

I think then RequestTextSurroundingSelection() is also a better name than saying
'Send...' (since that's an implementation detail that it sends a message).

[AKV, 2016-07-20 12:32:08]

PTAL!

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

https://codereview.chromium.org/2157793002/diff/1/chrome/browser/android/cont...
chrome/browser/android/contextualsearch/contextual_search_delegate.cc:247: if
(!focused_frame || focused_frame->HasTextSurroundingSelectionCallback()) {
On 2016/07/18 22:41:18, sievers wrote:
> Can you remove HasTextSurroundingSelectionCallback() from RenderFrameHost's
> interface and just handle this internally there?
> 
> I think then RequestTextSurroundingSelection() is also a better name than
saying
> 'Send...' (since that's an implementation detail that it sends a message).

Done.

[no sievers, 2016-07-20 21:14:32]

lgtm, thanks.

https://codereview.chromium.org/2157793002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2157793002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1578:
DCHECK(!callback.is_null());
nit: move the DCHECK() to the top since you run it above too.

https://codereview.chromium.org/2157793002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2157793002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:211: void
RequestTextSurroundingSelection(
Can you move this up into the block '// RenderFrameHost' where it has the
methods from that interface?

[AKV, 2016-07-20 21:54:44]

ajith.v@chromium.org changed reviewers:
+ tedchoc@chromium.org

[AKV, 2016-07-20 21:54:45]

@sievers - Thank you for reviewing. I will address the nits in next patch set.

+tedchoc to get OWNER lgtm for contextual_search_delegate.cc file

[Ted C, 2016-07-20 22:23:29]

lgtm w/ nit

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

https://codereview.chromium.org/2157793002/diff/20001/chrome/browser/android/...
chrome/browser/android/contextualsearch/contextual_search_delegate.cc:239:
DCHECK(content_view_core && content_view_core->GetWebContents());
this seems unneeded to me.  we weren't null checking cvc before

also, combining these into a single dcheck makes it hard to see what was
failing...was the cvc null or web contents?  I'm in favor of removing it all
together, but if not, please split into two.

[AKV, 2016-07-21 09:13:53]

Thank you Ted and Sievers for your reviews. I have incorporated the changes now.

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

https://codereview.chromium.org/2157793002/diff/20001/chrome/browser/android/...
chrome/browser/android/contextualsearch/contextual_search_delegate.cc:239:
DCHECK(content_view_core && content_view_core->GetWebContents());
On 2016/07/20 22:23:29, Ted C wrote:
> this seems unneeded to me.  we weren't null checking cvc before
> 
> also, combining these into a single dcheck makes it hard to see what was
> failing...was the cvc null or web contents?  I'm in favor of removing it all
> together, but if not, please split into two.

Done.

https://codereview.chromium.org/2157793002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2157793002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1578:
DCHECK(!callback.is_null());
On 2016/07/20 21:14:32, sievers wrote:
> nit: move the DCHECK() to the top since you run it above too.

Done.

https://codereview.chromium.org/2157793002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2157793002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:211: void
RequestTextSurroundingSelection(
On 2016/07/20 21:14:32, sievers wrote:
> Can you move this up into the block '// RenderFrameHost' where it has the
> methods from that interface?

Done.

[AKV, 2016-07-21 09:18:18]

The CQ bit was checked by ajith.v@chromium.org

[AKV, 2016-07-21 09:18:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/2157793002/#ps40001
(title: "Fixed the review comments.")

[commit-bot: I haz the power, 2016-07-21 09:18:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 10:24:08]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-21 10:27:11]

Description was changed from

==========
Remove ContentViewCore::RequestTextSurroundingSelection

This change takes care of scaling down the scope of CVC by
removing RequestTextSurroundingSelection() method. Logic has been
moved to RenderFrameHost. Additionally added the support of responding
with empty text when a new request comes during processing of a pending
request.

BUG=626770
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove ContentViewCore::RequestTextSurroundingSelection

This change takes care of scaling down the scope of CVC by
removing RequestTextSurroundingSelection() method. Logic has been
moved to RenderFrameHost. Additionally added the support of responding
with empty text when a new request comes during processing of a pending
request.

BUG=626770
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/deb84849f66aa9db12a4c4bd8eb0bb3e18fd441d
Cr-Commit-Position: refs/heads/master@{#406812}
==========

[commit-bot: I haz the power, 2016-07-21 10:27:12]

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