Upstream the Android port find-in-page feature.

Upstream the Android port find-in-page feature.

This patch provides the find-in-page Chromium code for the Android port.
Further code implementing the feature for Android WebView will be added later in a different patch.

Unlike the previous approach (CL 10699024) this patch uses the FindTabHelper class as Desktop Chrome does, introducing its new required APIs there. It also introduces FindMatchRects in a way symmetrical to the current Find/FindReply implementation in order to simplify any possible future adoption of this API by other platforms.


BUG=136762
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154893

[Leandro Graciá Gil, 2012-09-03 07:12:59]

joth: any Android-specific code, either Java or native.
jam: all the interactions with the content and chrome code plus owner approval.

Thanks!

[jam, 2012-09-03 16:50:15]

my initial reading of this is that there are two different code paths for
searching, one for android and the other for the rest. can't we combine these
two, i.e. make the existing chrome one expose the data that you need?

[Leandro Graciá Gil, 2012-09-04 06:00:22]

On 2012/09/03 16:50:15, John Abd-El-Malek wrote:
> my initial reading of this is that there are two different code paths for
> searching, one for android and the other for the rest. can't we combine these
> two, i.e. make the existing chrome one expose the data that you need?

These code paths are actually combined. Both Desktop and Android use the same
core messages (Find / FindReply), the same class handling the requests and the
results (FindTabHelper) and even the same find results notification
(NOTIFICATION_FIND_RESULT_AVAILABLE). What Android does extra is to provide the
find-in-page funcionality to the Java side so that the embedder app can make use
of it.

The other difference here is that Android has additional find-in-page messages
related to the results bar (the FindMatchRects stuff). This is because the
Android port allows going to specific results by tapping their markers.

[jam, 2012-09-04 16:39:09]

https://codereview.chromium.org/10905058/diff/1/chrome/android/java/src/org/c...
File
chrome/android/java/src/org/chromium/chrome/browser/FindNotificationDetails.java
(right):

https://codereview.chromium.org/10905058/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/FindNotificationDetails.java:13:
public class FindNotificationDetails {
question: do you need all these members for your application? if not, i wonder
if you even need to use the notification from FindTabHelper, or you can call the
java code directly when you get the WCD callback?

https://codereview.chromium.org/10905058/diff/1/chrome/browser/android/chrome...
File chrome/browser/android/chrome_web_contents_delegate_android.cc (right):

https://codereview.chromium.org/10905058/diff/1/chrome/browser/android/chrome...
chrome/browser/android/chrome_web_contents_delegate_android.cc:105:
OnFindResultAvailable(content::Source<WebContents>(source).ptr(),
nit: this indentation isn't according to style guide, i.e. the start of
parameters on each line has to be at the same column. also above and below

https://codereview.chromium.org/10905058/diff/1/chrome/browser/android/chrome...
chrome/browser/android/chrome_web_contents_delegate_android.cc:126: if
(!tab_contents || !tab_contents->find_tab_helper())
will this condition really happen?

https://codereview.chromium.org/10905058/diff/1/chrome/browser/android/chrome...
chrome/browser/android/chrome_web_contents_delegate_android.cc:147:
DCHECK(web_contents);
please get rid of these dchecks. if they're null, a crash will be just as
useful. littering the beginning of functions with dchecks isn't that helpful in
practice

https://codereview.chromium.org/10905058/diff/1/chrome/browser/android/chrome...
File chrome/browser/android/chrome_web_contents_delegate_android.h (right):

https://codereview.chromium.org/10905058/diff/1/chrome/browser/android/chrome...
chrome/browser/android/chrome_web_contents_delegate_android.h:45: 
nit: no blank lines between overridden methods

https://codereview.chromium.org/10905058/diff/1/chrome/browser/ui/find_bar/fi...
File chrome/browser/ui/find_bar/find_tab_helper.cc (right):

https://codereview.chromium.org/10905058/diff/1/chrome/browser/ui/find_bar/fi...
chrome/browser/ui/find_bar/find_tab_helper.cc:131: if (!find_op_aborted_ &&
!find_text_.empty()) {
here and below, are these checks really necessary? i.e. can we get to this point
through the UI if the find was aborted?

https://codereview.chromium.org/10905058/diff/1/chrome/browser/ui/find_bar/fi...
File chrome/browser/ui/find_bar/find_tab_helper.h (right):

https://codereview.chromium.org/10905058/diff/1/chrome/browser/ui/find_bar/fi...
chrome/browser/ui/find_bar/find_tab_helper.h:72: void
HandleGetFindMatchRectsReply(int version,
it seems very unnecessary that chrome_web_contents_delegate_android.cc calls
this android-only method, which right before it registered for the notification
which it fires, and then it listens to the notifications it fires and calls code
as a result. i.e. this method should be removed, and
chrome_web_contents_delegate_android.cc should just do what it does in response
to the notification inline

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

https://codereview.chromium.org/10905058/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_view_host_impl.h:326: virtual void
ActivateNearestFindResult(int request_id,
don't duplicate comments from interface in implementation classes. also,
overridden methods from an interface are always placed in the same order in the
derived classes

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

https://codereview.chromium.org/10905058/diff/1/content/common/view_messages....
content/common/view_messages.h:790: #if defined(OS_ANDROID)
this isn't the right order to put this message (ViewHostMsgs belong in the same
block, this is for browser->renderer messages)

https://codereview.chromium.org/10905058/diff/1/content/public/browser/web_co...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/10905058/diff/1/content/public/browser/web_co...
content/public/browser/web_contents_delegate.h:401: virtual void
GetFindMatchRectsReply(WebContents* web_contents,
nit: get rid of the "Get" in the method name, it's confusing because usually a
Get prefix means the method returns something. also, it's more consistent with
the above

[Leandro Graciá Gil, 2012-09-04 18:25:50]

https://codereview.chromium.org/10905058/diff/1/chrome/android/java/src/org/c...
File
chrome/android/java/src/org/chromium/chrome/browser/FindNotificationDetails.java
(right):

https://codereview.chromium.org/10905058/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/FindNotificationDetails.java:13:
public class FindNotificationDetails {
We need to use the notification from FindTabHelper as it holds the find-in-page
state and validates the replies. Also, we do need all of them in the Java side.

On 2012/09/04 16:39:09, John Abd-El-Malek wrote:
> question: do you need all these members for your application? if not, i wonder
> if you even need to use the notification from FindTabHelper, or you can call
the
> java code directly when you get the WCD callback?

https://codereview.chromium.org/10905058/diff/1/chrome/browser/android/chrome...
File chrome/browser/android/chrome_web_contents_delegate_android.cc (right):

https://codereview.chromium.org/10905058/diff/1/chrome/browser/android/chrome...
chrome/browser/android/chrome_web_contents_delegate_android.cc:105:
OnFindResultAvailable(content::Source<WebContents>(source).ptr(),
On 2012/09/04 16:39:09, John Abd-El-Malek wrote:
> nit: this indentation isn't according to style guide, i.e. the start of
> parameters on each line has to be at the same column. also above and below

Done.

https://codereview.chromium.org/10905058/diff/1/chrome/browser/android/chrome...
chrome/browser/android/chrome_web_contents_delegate_android.cc:126: if
(!tab_contents || !tab_contents->find_tab_helper())
On 2012/09/04 16:39:09, John Abd-El-Malek wrote:
> will this condition really happen?

Good catch. We actually removed some of these checks during the downstream
review, but this one seems to have slipped in.

https://codereview.chromium.org/10905058/diff/1/chrome/browser/android/chrome...
chrome/browser/android/chrome_web_contents_delegate_android.cc:147:
DCHECK(web_contents);
On 2012/09/04 16:39:09, John Abd-El-Malek wrote:
> please get rid of these dchecks. if they're null, a crash will be just as
> useful. littering the beginning of functions with dchecks isn't that helpful
in
> practice

Done.

https://codereview.chromium.org/10905058/diff/1/chrome/browser/android/chrome...
File chrome/browser/android/chrome_web_contents_delegate_android.h (right):

https://codereview.chromium.org/10905058/diff/1/chrome/browser/android/chrome...
chrome/browser/android/chrome_web_contents_delegate_android.h:45: 
On 2012/09/04 16:39:09, John Abd-El-Malek wrote:
> nit: no blank lines between overridden methods

Done.

https://codereview.chromium.org/10905058/diff/1/chrome/browser/ui/find_bar/fi...
File chrome/browser/ui/find_bar/find_tab_helper.cc (right):

https://codereview.chromium.org/10905058/diff/1/chrome/browser/ui/find_bar/fi...
chrome/browser/ui/find_bar/find_tab_helper.cc:131: if (!find_op_aborted_ &&
!find_text_.empty()) {
On 2012/09/04 16:39:09, John Abd-El-Malek wrote:
> here and below, are these checks really necessary? i.e. can we get to this
point
> through the UI if the find was aborted?

Probably not if we abort find (it should hide the result bar), but there is a
chance that the user taps on a result when the text is empty. This is because
the tickmarks (the match rects) are asynchronously updated.

https://codereview.chromium.org/10905058/diff/1/chrome/browser/ui/find_bar/fi...
File chrome/browser/ui/find_bar/find_tab_helper.h (right):

https://codereview.chromium.org/10905058/diff/1/chrome/browser/ui/find_bar/fi...
chrome/browser/ui/find_bar/find_tab_helper.h:72: void
HandleGetFindMatchRectsReply(int version,
On 2012/09/04 16:39:09, John Abd-El-Malek wrote:
> it seems very unnecessary that chrome_web_contents_delegate_android.cc calls
> this android-only method, which right before it registered for the
notification
> which it fires, and then it listens to the notifications it fires and calls
code
> as a result. i.e. this method should be removed, and
> chrome_web_contents_delegate_android.cc should just do what it does in
response
> to the notification inline

This was done this way in order to get a symmetrical design and behaviour with
the find results and have FindTabHelper handle both the requests and replies for
all find-in-page operations. Also, it seems that the idea of tapping to find
results to go to them caught some attention while upstreaming the WebKit side.
Because of that I tried to make this simple to be adopted by Desktop in the
future in case we decide so. Furthermore, we might require this in the future if
at some point this reply needs to check the FindTabHelper status.

However, as you point we need no state check for this reply so it should be
possible to call directly from ChromeWebContentsDelegateAndroid for now.

I'll be removing this from here in the next iteration of the patch. If you
change your mind just tell me and I'll bring it in again.

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

https://codereview.chromium.org/10905058/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_view_host_impl.h:326: virtual void
ActivateNearestFindResult(int request_id,
On 2012/09/04 16:39:09, John Abd-El-Malek wrote:
> don't duplicate comments from interface in implementation classes. also,
> overridden methods from an interface are always placed in the same order in
the
> derived classes

Done.

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

https://codereview.chromium.org/10905058/diff/1/content/common/view_messages....
content/common/view_messages.h:790: #if defined(OS_ANDROID)
On 2012/09/04 16:39:09, John Abd-El-Malek wrote:
> this isn't the right order to put this message (ViewHostMsgs belong in the
same
> block, this is for browser->renderer messages)

Done.

https://codereview.chromium.org/10905058/diff/1/content/public/browser/web_co...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/10905058/diff/1/content/public/browser/web_co...
content/public/browser/web_contents_delegate.h:401: virtual void
GetFindMatchRectsReply(WebContents* web_contents,
On 2012/09/04 16:39:09, John Abd-El-Malek wrote:
> nit: get rid of the "Get" in the method name, it's confusing because usually a
> Get prefix means the method returns something. also, it's more consistent with
> the above

Done.

[joth, 2012-09-04 22:43:31]

lgtm for the android files

https://codereview.chromium.org/10905058/diff/9002/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/10905058/diff/9002/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.h:32: #endif
nit: make the #include non-conditional if it's a cross-platform file (as it is
here)

https://codereview.chromium.org/10905058/diff/9002/content/common/view_messag...
File content/common/view_messages.h (right):

https://codereview.chromium.org/10905058/diff/9002/content/common/view_messag...
content/common/view_messages.h:54: #include "ui/gfx/rect_f.h"
(ditto)

[Leandro Graciá Gil, 2012-09-05 00:57:09]

https://codereview.chromium.org/10905058/diff/9002/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/10905058/diff/9002/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.h:32: #endif
On 2012/09/04 22:43:31, joth wrote:
> nit: make the #include non-conditional if it's a cross-platform file (as it is
> here)

Done.

https://codereview.chromium.org/10905058/diff/9002/content/common/view_messag...
File content/common/view_messages.h (right):

https://codereview.chromium.org/10905058/diff/9002/content/common/view_messag...
content/common/view_messages.h:54: #include "ui/gfx/rect_f.h"
On 2012/09/04 22:43:31, joth wrote:
> (ditto)

Done.

[jam, 2012-09-05 01:22:16]

lgtm

[commit-bot: I haz the power, 2012-09-05 01:23:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/10905058/9006

[commit-bot: I haz the power, 2012-09-05 03:46:38]

Change committed as 154893