[Android] Upstream the WebView find-in-page API implementation.

[Android] Upstream the WebView find-in-page API implementation.

This API requires implementing a browser -> renderer synchronous findAll message.
While this method is deprecated and an alternative asynchronous version is suggested
we still need to implement it for legacy WebView compatibility.


BUG=136762
TEST=3 new java tests.

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

[Leandro Graciá Gil, 2012-09-18 16:08:53]

joth: the android_webview code.
jam: the content code, which basically involves the synchronous IPC we talked a
few days ago.

Thanks!

[Leandro Graciá Gil, 2012-09-18 16:38:58]

I forgot joth was OOO. Marcin, could you please take a look to the WebView code
instead?

Thanks!

On 2012/09/18 16:08:53, Leandro Graciá Gil wrote:
> joth: the android_webview code.
> jam: the content code, which basically involves the synchronous IPC we talked
a
> few days ago.
> 
> Thanks!

[mkosiba (inactive), 2012-09-19 12:28:38]

LGTM for android_webview/

https://codereview.chromium.org/10941015/diff/1/android_webview/browser/DEPS
File android_webview/browser/DEPS (right):

https://codereview.chromium.org/10941015/diff/1/android_webview/browser/DEPS#...
android_webview/browser/DEPS:7:
"+third_party/WebKit/Source/WebKit/chromium/public/WebFindOptions.h",
nit: should be OK to simply say
"third_party/WebKit/Source/WebKit/chromium/public".

[Leandro Graciá Gil, 2012-09-19 12:35:09]

https://codereview.chromium.org/10941015/diff/1/android_webview/browser/DEPS
File android_webview/browser/DEPS (right):

https://codereview.chromium.org/10941015/diff/1/android_webview/browser/DEPS#...
android_webview/browser/DEPS:7:
"+third_party/WebKit/Source/WebKit/chromium/public/WebFindOptions.h",
On 2012/09/19 12:28:38, Martin Kosiba wrote:
> nit: should be OK to simply say
> "third_party/WebKit/Source/WebKit/chromium/public".

I discussed this with joth. The public folder contains not only POD structures
(which are ok in DEPS) but also exposes the API functionality (e.g. WebFrame,
WebView... which is not so good in DEPS). We're currently considering the
possibility of moving such POD structures to a separate folder in WebKit. For
now we decided to explicitly define this exception.

[jam, 2012-09-19 16:55:43]

https://codereview.chromium.org/10941015/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/10941015/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_process_host_impl.cc:460:
channel_.reset(new IPC::SyncChannel(
please make this conditional on webview (either runtime or compile time), so
that normal android builds don't get this

https://codereview.chromium.org/10941015/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/10941015/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_process_host_impl.h:300: // makes no sense
but SyncChannel still expects one. So, create a dummy event
why doesn't it make sense? seems that you'd want to set this when the user
attempts to quit (or the system asks you to?) so that the sync call can be
cancelled? or is this moot because there's no quitting on android?

https://codereview.chromium.org/10941015/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/10941015/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_view_host_impl.cc:512: Send(new
ViewMsg_SynchronousFind(GetRoutingID(), request_id, search_text,
can you make this conditional for webview as well?

https://codereview.chromium.org/10941015/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

https://codereview.chromium.org/10941015/diff/1/content/common/view_messages....
content/common/view_messages.h:1010: #if defined(OS_ANDROID)
nit: put all the android specific messages together in one ifdef block, at the
end of the declaration of the IPC messages (i.e. right before "Messages sent
from the renderer to the browser.")

https://codereview.chromium.org/10941015/diff/1/content/common/view_messages....
content/common/view_messages.h:1017: // this message should not be used for any
other purposes.
nit: this message _can't_ be used for any other purpose, so I think you want to
say is "don't use this as inspiration.".

https://codereview.chromium.org/10941015/diff/1/content/common/view_messages....
content/common/view_messages.h:1023: int /* Out: Number of occurrences found */,
nit: skip the "Out:". also, the convention is to put the parameter name inside
the comment, i.e.

int /* match_count */,

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

https://codereview.chromium.org/10941015/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:1059:
IPC_MESSAGE_HANDLER_DELAY_REPLY(ViewMsg_SynchronousFind, OnSynchronousFind)
nit: put all the android if-def'd handlers together (bonus points for doing the
same for the mac ones above too!)

https://codereview.chromium.org/10941015/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:3900: 
the moving of functions around made this file hard to review. can you put things
back and do cleanup separately?

https://codereview.chromium.org/10941015/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:4749:
DCHECK(!synchronous_find_pending_message_.get());
nit: make this a CHECK instead?

https://codereview.chromium.org/10941015/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/10941015/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.h:1121: void Find(int request_id, const
string16&, const WebKit::WebFindOptions&);
nit: google style is to always list the parameter names in headers

https://codereview.chromium.org/10941015/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.h:1433: scoped_ptr<IPC::Message>
synchronous_find_pending_message_;
nit: synchronous_find_pending_message_-> synchronous_find_reply_message_ to be
clearer

[Leandro Graciá Gil, 2012-09-19 18:08:59]

https://codereview.chromium.org/10941015/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/10941015/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_process_host_impl.cc:460:
channel_.reset(new IPC::SyncChannel(
On 2012/09/19 16:55:43, John Abd-El-Malek wrote:
> please make this conditional on webview (either runtime or compile time), so
> that normal android builds don't get this

We're currently working on this, but haven't found yet a proper way to implement
it since in this case it would need to be accessed from the content layer. This
kind of check will be needed for more features in the future, so we are very
interested on implementing this and doing it right. Any suggestions are very
welcome.

Since this shouldn't have any immediate impact and in any case it would be
limited to the Android port, would it be possible to leave it temporarily as
this until we have a proper check ready? We'd like to prevent this blocking the
whole patch, as it will probably make further WebView upstream patches harder to
prepare.

https://codereview.chromium.org/10941015/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/10941015/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_process_host_impl.h:300: // makes no sense
but SyncChannel still expects one. So, create a dummy event
On 2012/09/19 16:55:43, John Abd-El-Malek wrote:
> why doesn't it make sense? seems that you'd want to set this when the user
> attempts to quit (or the system asks you to?) so that the sync call can be
> cancelled? or is this moot because there's no quitting on android?

AFAIK there's no quitting on Android, we just get killed. Same goes for the
WebView case. I'll be updating the comment to make this clear.

https://codereview.chromium.org/10941015/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/10941015/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_view_host_impl.cc:512: Send(new
ViewMsg_SynchronousFind(GetRoutingID(), request_id, search_text,
On 2012/09/19 16:55:43, John Abd-El-Malek wrote:
> can you make this conditional for webview as well?

Sounds good to me. I'll do as soon as the check is ready.

https://codereview.chromium.org/10941015/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

https://codereview.chromium.org/10941015/diff/1/content/common/view_messages....
content/common/view_messages.h:1010: #if defined(OS_ANDROID)
On 2012/09/19 16:55:43, John Abd-El-Malek wrote:
> nit: put all the android specific messages together in one ifdef block, at the
> end of the declaration of the IPC messages (i.e. right before "Messages sent
> from the renderer to the browser.")

Done.

https://codereview.chromium.org/10941015/diff/1/content/common/view_messages....
content/common/view_messages.h:1017: // this message should not be used for any
other purposes.
On 2012/09/19 16:55:43, John Abd-El-Malek wrote:
> nit: this message _can't_ be used for any other purpose, so I think you want
to
> say is "don't use this as inspiration.".

Done.

https://codereview.chromium.org/10941015/diff/1/content/common/view_messages....
content/common/view_messages.h:1023: int /* Out: Number of occurrences found */,
On 2012/09/19 16:55:43, John Abd-El-Malek wrote:
> nit: skip the "Out:". also, the convention is to put the parameter name inside
> the comment, i.e.
> 
> int /* match_count */,

Done.

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

https://codereview.chromium.org/10941015/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:1059:
IPC_MESSAGE_HANDLER_DELAY_REPLY(ViewMsg_SynchronousFind, OnSynchronousFind)
On 2012/09/19 16:55:43, John Abd-El-Malek wrote:
> nit: put all the android if-def'd handlers together (bonus points for doing
the
> same for the mac ones above too!)

Done.

https://codereview.chromium.org/10941015/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:3900: 
On 2012/09/19 16:55:43, John Abd-El-Malek wrote:
> the moving of functions around made this file hard to review. can you put
things
> back and do cleanup separately?

Done.

https://codereview.chromium.org/10941015/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:4749:
DCHECK(!synchronous_find_pending_message_.get());
On 2012/09/19 16:55:43, John Abd-El-Malek wrote:
> nit: make this a CHECK instead?

Done.

https://codereview.chromium.org/10941015/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/10941015/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.h:1121: void Find(int request_id, const
string16&, const WebKit::WebFindOptions&);
On 2012/09/19 16:55:43, John Abd-El-Malek wrote:
> nit: google style is to always list the parameter names in headers

Done.

https://codereview.chromium.org/10941015/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.h:1433: scoped_ptr<IPC::Message>
synchronous_find_pending_message_;
On 2012/09/19 16:55:43, John Abd-El-Malek wrote:
> nit: synchronous_find_pending_message_-> synchronous_find_reply_message_ to be
> clearer

Done. Also grouped together the MACOSX messages as done with the message
handlers in render_view_impl.cc.

[jam, 2012-09-19 19:37:56]

https://codereview.chromium.org/10941015/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/10941015/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_process_host_impl.cc:460:
channel_.reset(new IPC::SyncChannel(
On 2012/09/19 18:08:59, Leandro Graciá Gil wrote:
> On 2012/09/19 16:55:43, John Abd-El-Malek wrote:
> > please make this conditional on webview (either runtime or compile time), so
> > that normal android builds don't get this
> 
> We're currently working on this, but haven't found yet a proper way to
implement
> it since in this case it would need to be accessed from the content layer.
This
> kind of check will be needed for more features in the future, so we are very
> interested on implementing this and doing it right. Any suggestions are very
> welcome.

I can't make any recommendations since I'm not familiar with Android.

> 
> Since this shouldn't have any immediate impact and in any case it would be
> limited to the Android port, would it be possible to leave it temporarily as
> this until we have a proper check ready? We'd like to prevent this blocking
the
> whole patch, as it will probably make further WebView upstream patches harder
to
> prepare.

I'm very wary of adding this without such a check. From experience, when things
like this land, it's easy for someone else without knowing that this shouldn't
be used to come and use it.

https://codereview.chromium.org/10941015/diff/9003/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/10941015/diff/9003/content/renderer/render_vi...
content/renderer/render_view_impl.cc:3896: // Check if this is a synchronous
find request.
nit: this comment and the one below are not very useful, i.e. they just explain
what the code does (see the style guide section on comments)

https://codereview.chromium.org/10941015/diff/9003/content/renderer/render_vi...
content/renderer/render_view_impl.cc:3946: -1, /* number_of_matches */
nit: can you skip the comments here? if someone wants to know what the
parameters are, they can look at the function

https://codereview.chromium.org/10941015/diff/9003/content/renderer/render_vi...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/10941015/diff/9003/content/renderer/render_vi...
content/renderer/render_view_impl.h:903: #if defined(OS_ANDROID)
move these below too

[Leandro Graciá Gil, 2012-09-19 20:34:04]

On 2012/09/19 19:37:56, John Abd-El-Malek wrote:
> I'm very wary of adding this without such a check. From experience, when
things
> like this land, it's easy for someone else without knowing that this shouldn't
> be used to come and use it.

What do you think about adding a new constructor, factory method or public flag
for WebView? The problem is that even if we implement a method to check if we
are in WebView it cannot be defined in the android_webview layer, but in content
due to the layering restrictions.

[Leandro Graciá Gil, 2012-09-20 16:05:05]

I've added a new command line switch to enable the WebView synchronous APIs. It
should be obvious enough to anybody that this is WebView-specific stuff.

https://codereview.chromium.org/10941015/diff/9003/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/10941015/diff/9003/content/renderer/render_vi...
content/renderer/render_view_impl.cc:3896: // Check if this is a synchronous
find request.
On 2012/09/19 19:37:56, John Abd-El-Malek wrote:
> nit: this comment and the one below are not very useful, i.e. they just
explain
> what the code does (see the style guide section on comments)

Done.

https://codereview.chromium.org/10941015/diff/9003/content/renderer/render_vi...
content/renderer/render_view_impl.cc:3946: -1, /* number_of_matches */
On 2012/09/19 19:37:56, John Abd-El-Malek wrote:
> nit: can you skip the comments here? if someone wants to know what the
> parameters are, they can look at the function

Done.

https://codereview.chromium.org/10941015/diff/9003/content/renderer/render_vi...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/10941015/diff/9003/content/renderer/render_vi...
content/renderer/render_view_impl.h:903: #if defined(OS_ANDROID)
On 2012/09/19 19:37:56, John Abd-El-Malek wrote:
> move these below too

Done. As in the other cases, grouped the MACOSX handlers too.

[Leandro Graciá Gil, 2012-09-20 16:07:20]

Marcin: there's a new change in the android_webview folder, the initialization
of the command line in webview_entry_point.cc. Everything else should be the
same.

[jam, 2012-09-21 00:57:23]

lgtm

On 2012/09/20 16:05:05, Leandro Graciá Gil wrote:
> I've added a new command line switch to enable the WebView synchronous APIs.
It
> should be obvious enough to anybody that this is WebView-specific stuff.

while this is better than nothing, it's not as strong of a check, because it can
be passed in the non-webview case to override this...

https://codereview.chromium.org/10941015/diff/13001/android_webview/native/aw...
File android_webview/native/aw_web_contents_delegate.h (right):

https://codereview.chromium.org/10941015/diff/13001/android_webview/native/aw...
android_webview/native/aw_web_contents_delegate.h:13: class Rect;
nit: a bit unnecessary since WebContentsDelegate has rects in it..

https://codereview.chromium.org/10941015/diff/13001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/10941015/diff/13001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:462: new
IPC::SyncChannel(
nit: need tabbing here

[mkosiba (inactive), 2012-09-21 09:10:10]

lgtm

[Leandro Graciá Gil, 2012-09-21 11:48:09]

At least the command line flag explicitly mentions WebView, so that it's clear
that it's not expected to be used outside it in the Android port. At the same
time we don't like the idea of a generic "IsWebView" check, especially in
content, since it can easily become the door to enable lots of hacks we'd prefer
not to have. We are happy to discuss this further and make any changes if you
like.

Also, I'll probably be landing this manually as I won't be able to work later
today and I'd prefer to be around when this lands in case I need to make any
unexpected fixes. If any of you has any comments, please tell them and I'll
prepare a follow-up patch.

https://codereview.chromium.org/10941015/diff/13001/android_webview/native/aw...
File android_webview/native/aw_web_contents_delegate.h (right):

https://codereview.chromium.org/10941015/diff/13001/android_webview/native/aw...
android_webview/native/aw_web_contents_delegate.h:13: class Rect;
On 2012/09/21 00:57:23, John Abd-El-Malek wrote:
> nit: a bit unnecessary since WebContentsDelegate has rects in it..

Done.

https://codereview.chromium.org/10941015/diff/13001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/10941015/diff/13001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:462: new
IPC::SyncChannel(
On 2012/09/21 00:57:23, John Abd-El-Malek wrote:
> nit: need tabbing here

Done.

[darin (slow to review), 2012-09-25 22:33:10]

Just a note from the peanut gallery:  I think there's a real opportunity here to
factor the find-in-page code out of RenderViewImpl and WebViewImpl.  Ideally,
we'd have some tear-off helper classes associated with both that can be used to
drive find-in-page.  I bet we could make the code a lot less complicated and
perhaps even isolate the OS-specific bits into separate source files.