mandoline: Add find in page.

mandoline: Add find in page.

This adds Ctrl+F as a keyboard shortcut to replace the current toolbar
with a find in page bar, where the user can enter some text.

BUG=496978
Committed: https://crrev.com/fe308b3359314164d5a74633dfbcf52c6367630b
Cr-Commit-Position: refs/heads/master@{#352535}

[Elliot Glaysher, 2015-09-28 21:22:56]

erg@chromium.org changed reviewers:
+ sky@chromium.org

[Elliot Glaysher, 2015-09-28 21:22:57]

https://codereview.chromium.org/1371773003/diff/100001/components/html_viewer...
File components/html_viewer/html_frame.cc (right):

https://codereview.chromium.org/1371773003/diff/100001/components/html_viewer...
components/html_viewer/html_frame.cc:843: void HTMLFrame::OnFind(int32
request_id, const mojo::String& search_text) {
This and GetFocusedElement() were copied almost verbatim from
content::RenderFrameImpl.

(The only changes here were webview() -> web_view(), and changing a helper
function to the two server_->... mojo calls towards the end.

https://codereview.chromium.org/1371773003/diff/100001/components/web_view/pu...
File components/web_view/public/interfaces/frame.mojom (right):

https://codereview.chromium.org/1371773003/diff/100001/components/web_view/pu...
components/web_view/public/interfaces/frame.mojom:118:
ReportFindInPageSelection(int32 request_id, int32 active_match_ordinal);
This sort of diverges from the chrome ipc version. In chrome ipc, we merge these
two messages into one message. Then at every time that it gets used minus one,
we stuff -1 in the payload of half the message. That is probably not great, so
clean this up in the mojom version.

[sky, 2015-09-28 23:18:59]

https://codereview.chromium.org/1371773003/diff/120001/components/html_viewer...
File components/html_viewer/html_frame.cc (right):

https://codereview.chromium.org/1371773003/diff/120001/components/html_viewer...
components/html_viewer/html_frame.cc:646: blink::WebDocument doc =
focused_frame->document();
What if focused_frame != web_frame_? Same comment for the loop below. I suspect
the looping needs to be implemented up in the server side.

https://codereview.chromium.org/1371773003/diff/120001/components/web_view/pu...
File components/web_view/public/interfaces/frame.mojom (right):

https://codereview.chromium.org/1371773003/diff/120001/components/web_view/pu...
components/web_view/public/interfaces/frame.mojom:177: OnFind(int32 request_id,
string search_text);
Since this and the next are meant to do something, nuke the On, e.g. Find and
StopFind. On the Frame side use the On, eg OnFind...

And document what request_id is.

[Elliot Glaysher, 2015-10-01 17:16:55]

ptal

This patch moves the logic for aggregation into WebView.

[sky, 2015-10-02 16:03:40]

https://codereview.chromium.org/1371773003/diff/240001/components/html_viewer...
File components/html_viewer/html_frame.cc (right):

https://codereview.chromium.org/1371773003/diff/240001/components/html_viewer...
components/html_viewer/html_frame.cc:689: if (focused_frame) {
Should you only look at focused_frame if it's the same as web_frame_?
I think you want something like the following:

HTMLFrame* frame = this;
while (frame) {
  if (web_view()) {
    if (web_view()->focusedFrame() == html_frame_) {
      // focus is in this frame.
    }
    return;  // Focus is in a different frame.
  }
  frame = frame->parent();
}

https://codereview.chromium.org/1371773003/diff/240001/components/html_viewer...
File components/html_viewer/html_frame.h (right):

https://codereview.chromium.org/1371773003/diff/240001/components/html_viewer...
components/html_viewer/html_frame.h:131: blink::WebView* web_view() const;
Having a const function return a non-const is bad. why do you need that?

https://codereview.chromium.org/1371773003/diff/240001/components/web_view/pu...
File components/web_view/public/interfaces/frame.mojom (right):

https://codereview.chromium.org/1371773003/diff/240001/components/web_view/pu...
components/web_view/public/interfaces/frame.mojom:183: (int32 request_id, string
search_text, bool found);
Does the callback really need a request_id too? Can't the source that invokes
Find() track the request_id if they care about?
Also, document what the expectations of this are. It isn't readily obvious why
there is both a callback and OnReportXX.

https://codereview.chromium.org/1371773003/diff/240001/components/web_view/pu...
components/web_view/public/interfaces/frame.mojom:189: ScopeStringMatches(int32
request_id, string search_test, bool reset);
What does 'Scope' mean in this context?

Could this function be merged with Find and Find gets arguments indicating the
type of find to do?

https://codereview.chromium.org/1371773003/diff/240001/components/web_view/pu...
File components/web_view/public/interfaces/web_view.mojom (right):

https://codereview.chromium.org/1371773003/diff/240001/components/web_view/pu...
components/web_view/public/interfaces/web_view.mojom:35:
ReportFindInPageSelection(int32 request_id, int32 active_match_ordinal);
What is active_match_ordinal? Given the api you're adding here, how does the
selection change?

https://codereview.chromium.org/1371773003/diff/240001/components/web_view/we...
File components/web_view/web_view_apptest.cc (right):

https://codereview.chromium.org/1371773003/diff/240001/components/web_view/we...
components/web_view/web_view_apptest.cc:255: 
nit: only one newline.

https://codereview.chromium.org/1371773003/diff/240001/components/web_view/we...
File components/web_view/web_view_impl.h (right):

https://codereview.chromium.org/1371773003/diff/240001/components/web_view/we...
components/web_view/web_view_impl.h:135: std::deque<Frame*>
pending_find_frames_;
Can the find related code be moved into it's own class that WebView owns?

[Elliot Glaysher, 2015-10-02 21:42:32]

erg@chromium.org changed reviewers:
+ paulmeyer@chromium.org, tkent@chromium.org

[Elliot Glaysher, 2015-10-02 21:42:33]

tkent: Hi, I'm trying to get find working in OOPIFs for Mandoline. I see that
you are an owner in blink and have reviewed the TextFinder code previously;
requesting OWNERS review of third_party/WebKit/. If you can think of any better
ways to do what I'm trying to do here, I'm all ears; I'm not a blink expert by
any means.

sky: ptal

paulmeyer: fyi, since he is going to do the content/ equivalent of this.

https://codereview.chromium.org/1371773003/diff/240001/components/html_viewer...
File components/html_viewer/html_frame.h (right):

https://codereview.chromium.org/1371773003/diff/240001/components/html_viewer...
components/html_viewer/html_frame.h:131: blink::WebView* web_view() const;
On 2015/10/02 16:03:40, sky wrote:
> Having a const function return a non-const is bad. why do you need that?

Deleted this and made GetFocusedElement() non const.

https://codereview.chromium.org/1371773003/diff/240001/components/web_view/pu...
File components/web_view/public/interfaces/frame.mojom (right):

https://codereview.chromium.org/1371773003/diff/240001/components/web_view/pu...
components/web_view/public/interfaces/frame.mojom:183: (int32 request_id, string
search_text, bool found);
On 2015/10/02 16:03:40, sky wrote:
> Does the callback really need a request_id too? Can't the source that invokes
> Find() track the request_id if they care about?
> Also, document what the expectations of this are. It isn't readily obvious why
> there is both a callback and OnReportXX.

I put that data in a Bind() on the mojo::WebView side of things, instead of
passing back the data in the response. Also greatly expanded documentation in
this file.

https://codereview.chromium.org/1371773003/diff/240001/components/web_view/pu...
components/web_view/public/interfaces/frame.mojom:189: ScopeStringMatches(int32
request_id, string search_test, bool reset);
On 2015/10/02 16:03:40, sky wrote:
> What does 'Scope' mean in this context?

It's an opaque name from blink. I totally agree with you that this name is
terrible.

I don't like it, but I think it's better to use consistent naming than to
diverge. (That's why the ReportFindIn... methods above are named such; they are
aping the blink names to make people from blink and content feel more at home.)

> Could this function be merged with Find and Find gets arguments indicating the
> type of find to do?

Sadly, no. Find and ScopeStringMatches do different things. Find is synchronous
while ScopeStringMatches is asynchronous. Find is about the currently matched
instance of the string while ScopeStringMatches is about all instances of the
string in the document.

[tkent, 2015-10-04 22:57:57]

third_party/WebKit lgtm.  This looks a reasonable change for OOPIF.

[sky, 2015-10-05 15:55:09]

https://codereview.chromium.org/1371773003/diff/280001/components/html_viewer...
File components/html_viewer/html_frame.cc (right):

https://codereview.chromium.org/1371773003/diff/280001/components/html_viewer...
components/html_viewer/html_frame.cc:894: const mojo::String& search_text,
What should happen if search_text is empty? Either DCHECK or early return?
Similar comment for 924.

https://codereview.chromium.org/1371773003/diff/280001/components/html_viewer...
components/html_viewer/html_frame.cc:917:
web_frame_->executeCommand(blink::WebString::fromUTF8("Unselect"),
What happens if Find() was invoked, something was selected, and then another
htmlframe gets focus? With the code as you have it GetFocusedElement() is going
to return an empty element. Will that clear out the focus selection?

https://codereview.chromium.org/1371773003/diff/280001/components/html_viewer...
File components/html_viewer/html_frame.h (right):

https://codereview.chromium.org/1371773003/diff/280001/components/html_viewer...
components/html_viewer/html_frame.h:253: blink::WebElement GetFocusedElement();
This should document it returns the focused element only if this frame has
focus.

https://codereview.chromium.org/1371773003/diff/280001/components/web_view/pu...
File components/web_view/public/interfaces/frame.mojom (right):

https://codereview.chromium.org/1371773003/diff/280001/components/web_view/pu...
components/web_view/public/interfaces/frame.mojom:116:
OnReportFindInFrameMatchCount(int32 request_id, int32 count,
Same comment about names from web_view.mojom here.

https://codereview.chromium.org/1371773003/diff/280001/components/web_view/pu...
components/web_view/public/interfaces/frame.mojom:181: // Searches for a given
string synchronously. If a match is found, it will be
The use of synchronous is confusing here given everything in async in mojo.

https://codereview.chromium.org/1371773003/diff/280001/components/web_view/pu...
components/web_view/public/interfaces/frame.mojom:187: // set, it will also make
the found text not selected.
'make the found text not selected' -> clear the selected find text.

https://codereview.chromium.org/1371773003/diff/280001/components/web_view/pu...
components/web_view/public/interfaces/frame.mojom:197: ScopeStringMatches(int32
request_id, string search_test, bool reset);
I would prefer you go with a name that doesn't assume knowledge of blink.
Something like FindAndSelectResults().

https://codereview.chromium.org/1371773003/diff/280001/components/web_view/pu...
File components/web_view/public/interfaces/web_view.mojom (right):

https://codereview.chromium.org/1371773003/diff/280001/components/web_view/pu...
components/web_view/public/interfaces/web_view.mojom:32:
ReportFindInPageMatchCount(int32 request_id, int32 count, bool final_update);
Why the Report? Maybe FindInPatchMatchCountUpdated and
FindInPageSelectionUpdated.

https://codereview.chromium.org/1371773003/diff/280001/components/web_view/pu...
components/web_view/public/interfaces/web_view.mojom:46: // Finds a string in
page.
Document what request_id is and any restrictions on it. Also, document what this
results in calling on WebViewClient.

[Elliot Glaysher, 2015-10-05 20:43:51]

Changed the documentation and the method names.

https://codereview.chromium.org/1371773003/diff/280001/components/html_viewer...
File components/html_viewer/html_frame.cc (right):

https://codereview.chromium.org/1371773003/diff/280001/components/html_viewer...
components/html_viewer/html_frame.cc:894: const mojo::String& search_text,
On 2015/10/05 15:55:08, sky wrote:
> What should happen if search_text is empty? Either DCHECK or early return?
> Similar comment for 924.

When search text is empty (this happens all the time), we go through the whole
process to return 0 of 0 back to the ui.

https://codereview.chromium.org/1371773003/diff/280001/components/html_viewer...
components/html_viewer/html_frame.cc:917:
web_frame_->executeCommand(blink::WebString::fromUTF8("Unselect"),
On 2015/10/05 15:55:08, sky wrote:
> What happens if Find() was invoked, something was selected, and then another
> htmlframe gets focus? With the code as you have it GetFocusedElement() is
going
> to return an empty element. Will that clear out the focus selection?

Added check to ensure focused element is part of web_frame_ before executing the
Unselect.

[sky, 2015-10-05 23:38:27]

Thanks for the improved names and doc. Much better.
LGTM

https://codereview.chromium.org/1371773003/diff/320001/components/web_view/pu...
File components/web_view/public/interfaces/frame.mojom (right):

https://codereview.chromium.org/1371773003/diff/320001/components/web_view/pu...
components/web_view/public/interfaces/frame.mojom:114: // Reports the number of
matches for a given find. This is an asynchronous
nit: update grammar 'this is an asynchronous notification can fire'?

https://codereview.chromium.org/1371773003/diff/320001/components/web_view/pu...
components/web_view/public/interfaces/frame.mojom:188: Find(int32 request_id,
string search_text) => (bool found);
Be aware that there are race conditions with using a callback, and invoking a
client function. By that I mean there is no guarantee that the callback will be
received before OnFindInFrame...

[Elliot Glaysher, 2015-10-05 23:42:30]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2015-10-05 23:42:30]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1371773003/#ps340001
(title: "Final documentation changes.")

[commit-bot: I haz the power, 2015-10-05 23:44:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1371773003/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1371773003/340001

[commit-bot: I haz the power, 2015-10-06 04:30:55]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2015-10-06 04:33:19]

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