Move find-in-page code from RenderView to RenderFrame.

This patch moves find-in-page code from RenderView to RenderFrame. This works towards the deprecation of RenderView, and is also the first step towards implementing multi-process find-in-page (for OOPIFs).

Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zhodPxMOxa8Bg4Wg0/edit?usp=sharing

BUG=457440
TEST=existing tests (no functional difference)

Committed: https://crrev.com/5d0a5f00b50ccc7590cc0b8e3cb62550b8d82e43
Cr-Commit-Position: refs/heads/master@{#370767}

[paulmeyer, 2016-01-18 21:32:53]

Description was changed from

==========
This patch moves find-in-page code from RenderView to RenderFrame. This works in
the direction of deprecation RenderView, and is also the first step towards
implementing multi-process find-in-page (for OOPIFs).

BUG=457440
==========

to

==========
This patch moves find-in-page code from RenderView to RenderFrame. This works in
the direction of deprecation RenderView, and is also the first step towards
implementing multi-process find-in-page (for OOPIFs).

Design doc for multi-process find-in-page:
https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh...

BUG=457440
TEST=no functional difference, so existing tests.
==========

[paulmeyer, 2016-01-18 21:33:22]

Description was changed from

==========
This patch moves find-in-page code from RenderView to RenderFrame. This works in
the direction of deprecation RenderView, and is also the first step towards
implementing multi-process find-in-page (for OOPIFs).

Design doc for multi-process find-in-page:
https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh...

BUG=457440
TEST=no functional difference, so existing tests.
==========

to

==========
This patch moves find-in-page code from RenderView to RenderFrame. This works in
the direction of deprecation RenderView, and is also the first step towards
implementing multi-process find-in-page (for OOPIFs).

Design doc for multi-process find-in-page:
https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh...

BUG=457440
TEST=existing tests (no functional difference)
==========

[paulmeyer, 2016-01-18 21:35:13]

paulmeyer@chromium.org changed reviewers:
+ nasko@chromium.org

[paulmeyer, 2016-01-18 21:35:13]

+nasko@

[paulmeyer, 2016-01-20 15:01:12]

Description was changed from

==========
This patch moves find-in-page code from RenderView to RenderFrame. This works in
the direction of deprecation RenderView, and is also the first step towards
implementing multi-process find-in-page (for OOPIFs).

Design doc for multi-process find-in-page:
https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh...

BUG=457440
TEST=existing tests (no functional difference)
==========

to

==========
This patch moves find-in-page code from RenderView to RenderFrame. This works
towards the deprecation of RenderView, and is also the first step towards
implementing multi-process find-in-page (for OOPIFs).

Design doc for multi-process find-in-page:
https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh...

BUG=457440
TEST=existing tests (no functional difference)
==========

[nasko, 2016-01-20 21:43:32]

This is an awesome start! Bunch of comments, but they are mostly minor and
stylistic.

https://codereview.chromium.org/1600953003/diff/1/content/common/frame_messag...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1600953003/diff/1/content/common/frame_messag...
content/common/frame_messages.h:1397: // Result of string search in the page.
s/page/frame/? Or document?

https://codereview.chromium.org/1600953003/diff/1/content/common/frame_messag...
content/common/frame_messages.h:1398: // Response to ViewMsg_Find with the
results of the requested find-in-page
s/ViewMsg/FrameMsg/

https://codereview.chromium.org/1600953003/diff/1/content/common/frame_messag...
content/common/frame_messages.h:1400: // coordinates) for the string found. If
|final_update| is false, it signals
Is the bit about "screen coordinates" still valid?

https://codereview.chromium.org/1600953003/diff/1/content/common/frame_messag...
content/common/frame_messages.h:1431: // frame traversal order starting in the
main frame, then by dom order.
Is the comment about frame traversal order still valid?

https://codereview.chromium.org/1600953003/diff/1/content/public/browser/rend...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/1600953003/diff/1/content/public/browser/rend...
content/public/browser/render_frame_host.h:196: // Asks the renderer to send the
rects of the current find matches.
nit: s/renderer/renderer process/.

https://codereview.chromium.org/1600953003/diff/1/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/1600953003/diff/1/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_instance_impl.cc:671:
render_frame_->set_plugin_find_handler(NULL);
nullptr in all new and changed code.

https://codereview.chromium.org/1600953003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1600953003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:5012: if (!is_main_frame_) {
Is this intentional? If so, it warrants a comment and/or TODO.

https://codereview.chromium.org/1600953003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:5992: if (!render_view_->webview())
Hmm, this method hasn't been converted to be RenderFrame based. Is this
intentional?

https://codereview.chromium.org/1600953003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:6009: int match_count,
Incorrect indentation. It will be easiest if you just run "git cl format" and
have the tools do the formatting work for you.

https://codereview.chromium.org/1600953003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/1600953003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.h:788: void OnFindMatchRects(int
current_version);
Would you do me a favor and move the #ifdef sections after all the other IPC
handlers? It should be at the end, but I've failed to enforce it on previous
reviews : (.

https://codereview.chromium.org/1600953003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.h:964: // Returns NULL if there is no such
WebPlugin.
s/NULL/null/

https://codereview.chromium.org/1600953003/diff/1/content/renderer/render_vie...
File content/renderer/render_view_impl.h (left):

https://codereview.chromium.org/1600953003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.h:112: struct WebFindOptions;
Is this still needed?

https://codereview.chromium.org/1600953003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.h:509: protected:
Why was "protected" removed? The following methods don't seem to be suitable to
be public.

[paulmeyer, 2016-01-20 22:54:15]

PTAL

https://codereview.chromium.org/1600953003/diff/1/content/common/frame_messag...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1600953003/diff/1/content/common/frame_messag...
content/common/frame_messages.h:1397: // Result of string search in the page.
On 2016/01/20 21:43:32, nasko wrote:
> s/page/frame/? Or document?

It will be the frame once traversal/aggregation is moved to the browser, but for
now it is still the page/document (assuming those are the same thing). I can
change to "document".

https://codereview.chromium.org/1600953003/diff/1/content/common/frame_messag...
content/common/frame_messages.h:1398: // Response to ViewMsg_Find with the
results of the requested find-in-page
On 2016/01/20 21:43:32, nasko wrote:
> s/ViewMsg/FrameMsg/

Done.

https://codereview.chromium.org/1600953003/diff/1/content/common/frame_messag...
content/common/frame_messages.h:1400: // coordinates) for the string found. If
|final_update| is false, it signals
On 2016/01/20 21:43:32, nasko wrote:
> Is the bit about "screen coordinates" still valid?

Yes.

https://codereview.chromium.org/1600953003/diff/1/content/common/frame_messag...
content/common/frame_messages.h:1431: // frame traversal order starting in the
main frame, then by dom order.
On 2016/01/20 21:43:32, nasko wrote:
> Is the comment about frame traversal order still valid?

Yes, for now. I could make it so that it is still valid after traversal is moved
to the browser as well.

https://codereview.chromium.org/1600953003/diff/1/content/public/browser/rend...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/1600953003/diff/1/content/public/browser/rend...
content/public/browser/render_frame_host.h:196: // Asks the renderer to send the
rects of the current find matches.
On 2016/01/20 21:43:32, nasko wrote:
> nit: s/renderer/renderer process/.

Ah... I've seen these used interchangeably all over. I didn't realize that there
was any preference. I've likely used both in a number of places in upcoming CLs
as well, but I'll try to change them all to render process.

https://codereview.chromium.org/1600953003/diff/1/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/1600953003/diff/1/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_instance_impl.cc:671:
render_frame_->set_plugin_find_handler(NULL);
On 2016/01/20 21:43:32, nasko wrote:
> nullptr in all new and changed code.

Done.

https://codereview.chromium.org/1600953003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1600953003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:5012: if (!is_main_frame_) {
On 2016/01/20 21:43:32, nasko wrote:
> Is this intentional? If so, it warrants a comment and/or TODO.

Done.

https://codereview.chromium.org/1600953003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:5992: if (!render_view_->webview())
On 2016/01/20 21:43:32, nasko wrote:
> Hmm, this method hasn't been converted to be RenderFrame based. Is this
> intentional?

No, I just missed that reference to |render_view_|. I've removed it now.

https://codereview.chromium.org/1600953003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:6009: int match_count,
On 2016/01/20 21:43:32, nasko wrote:
> Incorrect indentation. It will be easiest if you just run "git cl format" and
> have the tools do the formatting work for you.

Hmm... I usually only run "git cl format" when I get formatting messages during
"git cl upload". I'm surprised this one wasn't caught by that. I'll run it now.

https://codereview.chromium.org/1600953003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/1600953003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.h:788: void OnFindMatchRects(int
current_version);
On 2016/01/20 21:43:32, nasko wrote:
> Would you do me a favor and move the #ifdef sections after all the other IPC
> handlers? It should be at the end, but I've failed to enforce it on previous
> reviews : (.

Done.

https://codereview.chromium.org/1600953003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.h:964: // Returns NULL if there is no such
WebPlugin.
On 2016/01/20 21:43:32, nasko wrote:
> s/NULL/null/

Did you mean "nullptr"?

https://codereview.chromium.org/1600953003/diff/1/content/renderer/render_vie...
File content/renderer/render_view_impl.h (left):

https://codereview.chromium.org/1600953003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.h:112: struct WebFindOptions;
On 2016/01/20 21:43:32, nasko wrote:
> Is this still needed?

No. Good catch.

https://codereview.chromium.org/1600953003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.h:509: protected:
On 2016/01/20 21:43:32, nasko wrote:
> Why was "protected" removed? The following methods don't seem to be suitable
to
> be public.

Hahahaha, I did this long enough ago that I didn't remember doing it, and I was
super confused when I saw this. I couldn't figure out why I would ever just go
and remove the protected label out of a class. After revealing a number of lines
above this though, I noticed why. As it turns out, it was because these methods
are ALREADY protected. :P

[nasko, 2016-01-20 23:38:26]

LGTM once the issue in render_frame_impl.cc is fixed.

https://codereview.chromium.org/1600953003/diff/1/content/public/browser/rend...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/1600953003/diff/1/content/public/browser/rend...
content/public/browser/render_frame_host.h:196: // Asks the renderer to send the
rects of the current find matches.
On 2016/01/20 22:54:14, Paul Meyer wrote:
> On 2016/01/20 21:43:32, nasko wrote:
> > nit: s/renderer/renderer process/.
> 
> Ah... I've seen these used interchangeably all over. I didn't realize that
there
> was any preference. I've likely used both in a number of places in upcoming
CLs
> as well, but I'll try to change them all to render process.

The rule of thumb we try to use is "renderer process" for genericly referring to
anything running in the renderer process. Otherwise the proper object name
should be used, RenderFrame/RenderFrameProxy/etc.

https://codereview.chromium.org/1600953003/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/1600953003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.h:788: void OnFindMatchRects(int
current_version);
On 2016/01/20 22:54:15, Paul Meyer wrote:
> On 2016/01/20 21:43:32, nasko wrote:
> > Would you do me a favor and move the #ifdef sections after all the other IPC
> > handlers? It should be at the end, but I've failed to enforce it on previous
> > reviews : (.
> 
> Done.

Thanks!

https://codereview.chromium.org/1600953003/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.h:964: // Returns NULL if there is no such
WebPlugin.
On 2016/01/20 22:54:15, Paul Meyer wrote:
> On 2016/01/20 21:43:32, nasko wrote:
> > s/NULL/null/
> 
> Did you mean "nullptr"?

I'm fine with "null" in comments as it is the concept of nullness. nullptr works
too, as it is concrete value : ).

https://codereview.chromium.org/1600953003/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1600953003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:5996: if (frame_->isWebLocalFrame() &&
frame_->document().isPluginDocument())
frame_ will always be WebLocalFrame. No need to check that. It was needed
before, as the main frame is not guaranteed to be WebLocalFrame, in cases this
is a subframe.

[Charlie Reis, 2016-01-21 00:02:27]

creis@chromium.org changed reviewers:
+ creis@chromium.org

[Charlie Reis, 2016-01-21 00:02:28]

Nice!  Drive-by LGTM with a few nits.

https://codereview.chromium.org/1600953003/diff/20001/content/public/browser/...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/1600953003/diff/20001/content/public/browser/...
content/public/browser/render_frame_host.h:196: // Asks the render process to
send the rects of the current find matches.
nit: renderer process

https://codereview.chromium.org/1600953003/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1600953003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:4997: // don't leave text selected as you
move to the next frame.
nit: Don't

https://codereview.chromium.org/1600953003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:5003: // What is the next frame to search?
(we might be going backwards). Note
nit: We

https://codereview.chromium.org/1600953003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:5036: int ordinal = result ? -1 : 0;  //
-1 here means, we might know more later.
nit: Drop comma.

https://codereview.chromium.org/1600953003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:5046: // Scoping effort begins, starting
with the mainframe.
nit: main frame.

https://codereview.chromium.org/1600953003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:5124: // to rate-limiting).
nit: Drop extra close paren.

https://codereview.chromium.org/1600953003/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/1600953003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.h:293: // Get/set the plugin which will be
used as to handle document find requests.
nit: Drop "as"?

[Charlie Reis, 2016-01-21 00:03:50]

Also +site-isolation-reviews@.  (Would you mind CC'ing that list on future OOPIF
CLs?  It helps us track the work we're doing as a team.)

[paulmeyer, 2016-01-21 14:25:46]

https://codereview.chromium.org/1600953003/diff/20001/content/public/browser/...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/1600953003/diff/20001/content/public/browser/...
content/public/browser/render_frame_host.h:196: // Asks the render process to
send the rects of the current find matches.
On 2016/01/21 00:02:28, Charlie Reis wrote:
> nit: renderer process

Done.

https://codereview.chromium.org/1600953003/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1600953003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:4997: // don't leave text selected as you
move to the next frame.
On 2016/01/21 00:02:28, Charlie Reis wrote:
> nit: Don't

Done.

https://codereview.chromium.org/1600953003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:5003: // What is the next frame to search?
(we might be going backwards). Note
On 2016/01/21 00:02:28, Charlie Reis wrote:
> nit: We

Done.

https://codereview.chromium.org/1600953003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:5036: int ordinal = result ? -1 : 0;  //
-1 here means, we might know more later.
On 2016/01/21 00:02:28, Charlie Reis wrote:
> nit: Drop comma.

Done.

https://codereview.chromium.org/1600953003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:5046: // Scoping effort begins, starting
with the mainframe.
On 2016/01/21 00:02:28, Charlie Reis wrote:
> nit: main frame.

Done.

https://codereview.chromium.org/1600953003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:5124: // to rate-limiting).
On 2016/01/21 00:02:28, Charlie Reis wrote:
> nit: Drop extra close paren.

Done.

https://codereview.chromium.org/1600953003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:5996: if (frame_->isWebLocalFrame() &&
frame_->document().isPluginDocument())
On 2016/01/20 23:38:26, nasko wrote:
> frame_ will always be WebLocalFrame. No need to check that. It was needed
> before, as the main frame is not guaranteed to be WebLocalFrame, in cases this
> is a subframe.

Done.

https://codereview.chromium.org/1600953003/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/1600953003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.h:293: // Get/set the plugin which will be
used as to handle document find requests.
On 2016/01/21 00:02:28, Charlie Reis wrote:
> nit: Drop "as"?

Done. In my defense, I didn't write all these bad grammar comments. :)

[paulmeyer, 2016-01-21 14:31:20]

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

[paulmeyer, 2016-01-21 14:31:20]

+sky@ for OWNER review of find_tab_helper.cc

[sky, 2016-01-21 18:54:31]

LGTM

[paulmeyer, 2016-01-21 18:55:52]

The CQ bit was checked by paulmeyer@chromium.org

[paulmeyer, 2016-01-21 18:55:53]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1600953003/#ps40001
(title: "Addressed final comments by nasko@, creis@.")

[commit-bot: I haz the power, 2016-01-21 18:57:52]

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

[commit-bot: I haz the power, 2016-01-21 20:15:59]

Description was changed from

==========
This patch moves find-in-page code from RenderView to RenderFrame. This works
towards the deprecation of RenderView, and is also the first step towards
implementing multi-process find-in-page (for OOPIFs).

Design doc for multi-process find-in-page:
https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh...

BUG=457440
TEST=existing tests (no functional difference)
==========

to

==========
This patch moves find-in-page code from RenderView to RenderFrame. This works
towards the deprecation of RenderView, and is also the first step towards
implementing multi-process find-in-page (for OOPIFs).

Design doc for multi-process find-in-page:
https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh...

BUG=457440
TEST=existing tests (no functional difference)
==========

[commit-bot: I haz the power, 2016-01-21 20:16:00]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-21 20:17:26]

Description was changed from

==========
This patch moves find-in-page code from RenderView to RenderFrame. This works
towards the deprecation of RenderView, and is also the first step towards
implementing multi-process find-in-page (for OOPIFs).

Design doc for multi-process find-in-page:
https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh...

BUG=457440
TEST=existing tests (no functional difference)
==========

to

==========
This patch moves find-in-page code from RenderView to RenderFrame. This works
towards the deprecation of RenderView, and is also the first step towards
implementing multi-process find-in-page (for OOPIFs).

Design doc for multi-process find-in-page:
https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh...

BUG=457440
TEST=existing tests (no functional difference)

Committed: https://crrev.com/5d0a5f00b50ccc7590cc0b8e3cb62550b8d82e43
Cr-Commit-Position: refs/heads/master@{#370767}
==========

[commit-bot: I haz the power, 2016-01-21 20:17:28]

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