Plumbing for browser process to access text surrounding selection.

Plumbing for browser process to access text surrounding selection.

This is using cl 294073005 in order for Blink to give that information
to the embedder. The information is passed asynchronously with an IPC
message for the request and another for the response.

BUG=330238
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277474

[mlamouri (slow - plz ping), 2014-05-21 20:45:43]

jdduke, could you review the changes in:
  content/public/android/
  content/browser/android/

dcheng, could your review the changes in:
  content/common/

jochen, could you review the rest of the plumbing.

Thanks all :)

[dcheng, 2014-05-21 21:05:38]

At a high level, it seems weird that RenderFrameHostImpl is just forwarding
things to RenderViewHostImpl. I've added nasko@ to the review, since he's much
more involved with the browser side site isolation changes.

Also, is it guaranteed in this case that the focused frame will be the one with
the selection?

https://codereview.chromium.org/292113008/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/292113008/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_base.h:321: unsigned
end_offset) {};
Is there a reason this isn't pure virtual like most of the other methods?
Consistency would be nice.

https://codereview.chromium.org/292113008/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/292113008/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2476:
String content, int startOffset, int endOffset) {
This is unsigned coming back from the renderer. Why are the integer types not
consistent?

https://codereview.chromium.org/292113008/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3227:
long nativeContentViewCoreImpl, int maxLength);
Similarly, this starts out as an int here... but ends up as an unsigned int in
RenderFrameHostImpl's IPC message handler. This inconsistency seems trange.

https://codereview.chromium.org/292113008/diff/1/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/292113008/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.cc:1137:
IPC_MESSAGE_HANDLER(ViewMsg_TextAroundSelection, OnTextAroundSelection)
Presumably this is a new IPC message, but I don't see it defined anywhere.

[mlamouri (slow - plz ping), 2014-05-21 21:19:39]

I'm using RenderFrame{Host,} because I'm accessing a WebFrame API on the
renderer side. With RenderView{Host,}, on the renderer, I would get the focused
frame and call the WebFrame API on it. Here, it happens that ContentViewCore
wants to know about it so it sounds fair to go directly to the view. Though,
this is mostly guts feeling, I would love to hear about people who actually
know.

Regarding the selection, I'm not sure I understand your question. AFAIUI a frame
is focused iff it has the selection.

https://codereview.chromium.org/292113008/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/292113008/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_base.h:321: unsigned
end_offset) {};
On 2014/05/21 21:05:38, dcheng wrote:
> Is there a reason this isn't pure virtual like most of the other methods?
> Consistency would be nice.

Making it pure virtual would require to have empty definitions in every single
implementation. That sounds unfortunate. I could do it if needed. Note that some
methods have empty bodies in this class.

https://codereview.chromium.org/292113008/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/292113008/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2476:
String content, int startOffset, int endOffset) {
On 2014/05/21 21:05:38, dcheng wrote:
> This is unsigned coming back from the renderer. Why are the integer types not
> consistent?

Because Java doesn't know about unsigned int.

https://codereview.chromium.org/292113008/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3227:
long nativeContentViewCoreImpl, int maxLength);
On 2014/05/21 21:05:38, dcheng wrote:
> Similarly, this starts out as an int here... but ends up as an unsigned int in
> RenderFrameHostImpl's IPC message handler. This inconsistency seems trange.

ditto.

https://codereview.chromium.org/292113008/diff/1/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/292113008/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.cc:1137:
IPC_MESSAGE_HANDLER(ViewMsg_TextAroundSelection, OnTextAroundSelection)
On 2014/05/21 21:05:38, dcheng wrote:
> Presumably this is a new IPC message, but I don't see it defined anywhere.

This is something I forgot to remove when cleaning up the patch.

[nasko, 2014-05-22 18:59:04]

https://codereview.chromium.org/292113008/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/292113008/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2476:
String content, int startOffset, int endOffset) {
On 2014/05/21 21:19:40, Mounir Lamouri wrote:
> On 2014/05/21 21:05:38, dcheng wrote:
> > This is unsigned coming back from the renderer. Why are the integer types
not
> > consistent?
> 
> Because Java doesn't know about unsigned int.

Should there be a safe conversion somewhere on the path of this value then? What
if a compromised renderer sends a big unsigned int that corresponds to negative
signed int? This should be a failure condition somewhere.

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

https://codereview.chromium.org/292113008/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:702:
render_view_host_->OnTextSurroundingSelectionResponse(
It will be better to avoid forwarding new functionality to RVH, as it will
disappear at some point in the future.
Each RFH will have an associated view, so it will be better to introduce a
GetView() method and for now it will get it from RVH, but in the future will use
its own.

https://codereview.chromium.org/292113008/diff/20001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/292113008/diff/20001/content/common/frame_mes...
content/common/frame_messages.h:364: unsigned /* max_length */)
nit: unsigned what? It is best to be explicit about types. It should be changed
in all files.

[dcheng, 2014-05-22 19:02:36]

https://codereview.chromium.org/292113008/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/292113008/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2476:
String content, int startOffset, int endOffset) {
On 2014/05/21 21:19:40, Mounir Lamouri wrote:
> On 2014/05/21 21:05:38, dcheng wrote:
> > This is unsigned coming back from the renderer. Why are the integer types
not
> > consistent?
> 
> Because Java doesn't know about unsigned int.

Right... so I think this should just be int all the way through. Several other
indices related to selections in the Blink API layer are 'int' as well, so it
keeps things consistent.

[mlamouri (slow - plz ping), 2014-05-29 15:23:12]

On 2014/05/22 19:02:36, dcheng wrote:
>
https://codereview.chromium.org/292113008/diff/1/content/public/android/java/...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> (right):
> 
>
https://codereview.chromium.org/292113008/diff/1/content/public/android/java/...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2476:
> String content, int startOffset, int endOffset) {
> On 2014/05/21 21:19:40, Mounir Lamouri wrote:
> > On 2014/05/21 21:05:38, dcheng wrote:
> > > This is unsigned coming back from the renderer. Why are the integer types
> not
> > > consistent?
> > 
> > Because Java doesn't know about unsigned int.
> 
> Right... so I think this should just be int all the way through. Several other
> indices related to selections in the Blink API layer are 'int' as well, so it
> keeps things consistent.

It happens that the SurroundingText class is using unsigned :(

[mlamouri (slow - plz ping), 2014-05-29 16:35:03]

I've updated the plumbing code to handle the type conversion. I will see with
the folks owning the Blink code if they want to keep unsigned or move to int.

[mlamouri (slow - plz ping), 2014-06-03 12:40:46]

review ping

[nasko, 2014-06-03 21:02:23]

You still need to qualify that it is "unsigned int", which is the chromium
style.

[dcheng, 2014-06-03 21:54:55]

What have other places that need to bring an unsigned int into Java done? This
can't be the first time this has been needed.

[mlamouri (slow - plz ping), 2014-06-04 14:38:54]

nasko@, I've updated the patch to use "unsigned int" instead of "unsigned".

dcheng@, I checked and asked around and it seems that we just don't usually get
unsigned up to Java. Not per design as far as my team is aware but mostly
because that does not happen. No one seem to be aware of a reason to not do that
so unless the Blink API changes to use 'int', using 'unsigned int' should be
fine.

jdduke@, WDYT?

PTAL :)

[jdduke (slow), 2014-06-04 15:12:15]

On 2014/06/04 14:38:54, Mounir Lamouri wrote:
> nasko@, I've updated the patch to use "unsigned int" instead of "unsigned".
> 
> dcheng@, I checked and asked around and it seems that we just don't usually
get
> unsigned up to Java. Not per design as far as my team is aware but mostly
> because that does not happen. No one seem to be aware of a reason to not do
that
> so unless the Blink API changes to use 'int', using 'unsigned int' should be
> fine.
> 
> jdduke@, WDYT?
> 
> PTAL :)

Honestly, I think we should be consistent with existing WebFrame APIs that use
signed integers when dealing with selection offsets.  Can you make that change
to your proposed addition to WebFrame?

[mlamouri (slow - plz ping), 2014-06-05 16:33:02]

I've changed the WebFrame API to use int even though the internal Blink API uses
unsigned. This CL has been updated to use only int so hopefully the int vs
unsigned debate will move to the Blink CLs.

PTAL :)

[jdduke (slow), 2014-06-06 01:28:32]

https://codereview.chromium.org/292113008/diff/80001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/292113008/diff/80001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:1548:
Java_ContentViewCore_onTextSurroundingSelectionResponse(
I believe Donn is exploring ways to route the string to downstream code without
passing it to Java.  For now, let's leave out the wiring in
ContentViewCore/ContentViewCoreImpl, and have it dead-end in RWHVAndroid.  In
the follow-up patch, where we hook it up to a public consumer/client, we can add
the wiring back as necessary.

[jdduke (slow), 2014-06-06 01:29:57]

On 2014/06/06 01:28:32, jdduke wrote:
>
https://codereview.chromium.org/292113008/diff/80001/content/browser/android/...
> File content/browser/android/content_view_core_impl.cc (right):
> 
>
https://codereview.chromium.org/292113008/diff/80001/content/browser/android/...
> content/browser/android/content_view_core_impl.cc:1548:
> Java_ContentViewCore_onTextSurroundingSelectionResponse(
> I believe Donn is exploring ways to route the string to downstream code
without
> passing it to Java.  For now, let's leave out the wiring in
> ContentViewCore/ContentViewCoreImpl, and have it dead-end in RWHVAndroid.  In
> the follow-up patch, where we hook it up to a public consumer/client, we can
add
> the wiring back as necessary.

Sorry, I didn't realize that was the case until Donn told me after our meeting
this morning (might have saved some trouble with the signed/unsigned business,
sigh, but at least we'll have an easier time wiring it up to Java should we need
to in the future).

[donnd, 2014-06-06 02:11:21]

Jared, thanks for pointing this out, and sorry I didn't realize this either.

[nasko, 2014-06-06 17:10:22]

On 2014/06/06 02:11:21, Donn wrote:
> Jared, thanks for pointing this out, and sorry I didn't realize this either.

I'm confused by the last comments. Is this CL still valid and need to be
reviewed or not?

[jdduke (slow), 2014-06-06 17:14:28]

On 2014/06/06 17:10:22, nasko wrote:
> On 2014/06/06 02:11:21, Donn wrote:
> > Jared, thanks for pointing this out, and sorry I didn't realize this either.
> 
> I'm confused by the last comments. Is this CL still valid and need to be
> reviewed or not?

Yes, it's still valid, but the Java-side hooks are not immediately necessary.

[nasko, 2014-06-06 17:25:19]

IPC LGTM

[dcheng, 2014-06-06 17:34:56]

Err, are we still coercing unsigned to signed?

[mlamouri (slow - plz ping), 2014-06-07 12:53:20]

On 2014/06/06 17:34:56, dcheng wrote:
> Err, are we still coercing unsigned to signed?

This CL is only using signed integers.

[mlamouri (slow - plz ping), 2014-06-10 13:19:05]

I have updated the CL to apply on top of the latest changes from the Blink API
changes (change WebSurroundingText API layer instead of WebFrame).

PTAL.

[jdduke (slow), 2014-06-10 14:53:32]

https://codereview.chromium.org/292113008/diff/100001/content/browser/android...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/292113008/diff/100001/content/browser/android...
content/browser/android/content_view_core_impl.cc:1538: void
ContentViewCoreImpl::OnTextSurroundingSelectionResponse(
As mentioned, let's defer adding the hooks into Java for now.  Donn can add them
as necessary when we actually expose the added methods publicly to an embedder
(the choice between a C++ or Java embedder has not yet been made). It should be
sufficient to simply drop the changes to ContentViewCore and
ContentViewCoreImpl, and leave an empty stub in RWHVAndroid.

[dcheng, 2014-06-10 18:06:51]

On 2014/06/07 at 12:53:20, mlamouri wrote:
> On 2014/06/06 17:34:56, dcheng wrote:
> > Err, are we still coercing unsigned to signed?
> 
> This CL is only using signed integers.

Right, but at the end of the day, we're still casting from unsigned to int--it
just so happens that this case is hidden inside Blink now. So we still have an
issue when the unsigned doesn't fit in an int...

[jdduke (slow), 2014-06-12 16:23:15]

On 2014/06/10 18:06:51, dcheng wrote:
> On 2014/06/07 at 12:53:20, mlamouri wrote:
> > On 2014/06/06 17:34:56, dcheng wrote:
> > > Err, are we still coercing unsigned to signed?
> > 
> > This CL is only using signed integers.
> 
> Right, but at the end of the day, we're still casting from unsigned to int--it
> just so happens that this case is hidden inside Blink now. So we still have an
> issue when the unsigned doesn't fit in an int...

It appears the Blink API has been modified to use a size_t (I think we just need
a rebase), in which case we should be able to propagate the value to the browser
without any casting.  Note that we'll no longer be passing these values to Java,
so we shouldn't be concerned about a cast there either.  Note that this is a
time-critical feature, so if that solution doesn't work for you, please contact
me directly dcheng@ so we can work something out ASAP.

https://codereview.chromium.org/292113008/diff/120001/content/browser/android...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/292113008/diff/120001/content/browser/android...
content/browser/android/content_view_core_impl.cc:39: #include
"content/browser/web_contents/web_contents_view_android.h"
Nit: Looks like you can get rid of this change.

[mlamouri (slow - plz ping), 2014-06-12 17:50:45]

I switch the entire CL to use size_t so there is no type conversion. I've also
applied jared's nit wrt to the #include.

PTAL.

[dcheng, 2014-06-12 21:13:20]

On 2014/06/12 at 16:23:15, jdduke wrote:
> On 2014/06/10 18:06:51, dcheng wrote:
> > On 2014/06/07 at 12:53:20, mlamouri wrote:
> > > On 2014/06/06 17:34:56, dcheng wrote:
> > > > Err, are we still coercing unsigned to signed?
> > > 
> > > This CL is only using signed integers.
> > 
> > Right, but at the end of the day, we're still casting from unsigned to
int--it
> > just so happens that this case is hidden inside Blink now. So we still have
an
> > issue when the unsigned doesn't fit in an int...
> 
> It appears the Blink API has been modified to use a size_t (I think we just
need a rebase), in which case we should be able to propagate the value to the
browser without any casting.  Note that we'll no longer be passing these values
to Java, so we shouldn't be concerned about a cast there either.  Note that this
is a time-critical feature, so if that solution doesn't work for you, please
contact me directly dcheng@ so we can work something out ASAP.

"time-critical feature" -- this statement makes me sad.

> 
>
https://codereview.chromium.org/292113008/diff/120001/content/browser/android...
> File content/browser/android/content_view_core_impl.cc (right):
> 
>
https://codereview.chromium.org/292113008/diff/120001/content/browser/android...
> content/browser/android/content_view_core_impl.cc:39: #include
"content/browser/web_contents/web_contents_view_android.h"
> Nit: Looks like you can get rid of this change.

IPC changes LGTM.

[mlamouri (slow - plz ping), 2014-06-12 21:16:17]

jam@, could you review the content/ plumbing?

[jdduke (slow), 2014-06-12 21:23:00]

On 2014/06/12 21:13:20, dcheng wrote:
> "time-critical feature" -- this statement makes me sad.

Not my words, either =/

[jam, 2014-06-13 17:05:58]

redirecting to Avi

[Avi (use Gerrit), 2014-06-15 01:44:14]

lgtm

[mlamouri (slow - plz ping), 2014-06-16 10:38:43]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-06-16 10:39:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlamouri@chromium.org/292113008/160001

[commit-bot: I haz the power, 2014-06-16 17:22:28]

Change committed as 277474