Allow users to close the find session and activate the current link via ctrl-...

Allow users to close the find session and activate the current link via ctrl-enter.

this only hooks up ctrl-enter on gtk for now, but adding it on windows/mac should be trivial

webkit side, which needs to land first, is here:
https://bugs.webkit.org/show_bug.cgi?id=35407

This also enables the FindInPageControllerTest tests on linux/gtk.

BUG=29500
TEST=FindInPageControllerTest.ActivateLinkNavigatesPage


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

[Evan Stade, 2010-03-06 01:08:16]

Ignore the webkit changes; I only included them for the sake of the trybots.

[tony, 2010-03-08 00:24:26]

How hard would it be to write an interactive UI test for this?

http://codereview.chromium.org/660137/diff/2001/2011
File chrome/browser/cocoa/find_bar_cocoa_controller.mm (right):

http://codereview.chromium.org/660137/diff/2001/2011#newcode138
chrome/browser/cocoa/find_bar_cocoa_controller.mm:138:
tab_contents->StopFinding(true, false);  // true = clear selection on page.
The comment here is now confusing.  Maybe just remove the comment?  It might be
nice to make enums for this, but I don't feel strongly about it.

http://codereview.chromium.org/660137/diff/2001/2004
File chrome/browser/find_bar_controller.cc (right):

http://codereview.chromium.org/660137/diff/2001/2004#newcode46
chrome/browser/find_bar_controller.cc:46: tab_contents_->StopFinding(false,
true);
Do you mean to pass activate as the second param here?  You're not using
activate anywhere.

http://codereview.chromium.org/660137/diff/2001/2010
File chrome/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/660137/diff/2001/2010#newcode207
chrome/browser/renderer_host/render_view_host.h:207: // Cancel a pending find
operation.
Nit: Can you keep the comment about clear_selection and add a sentence
describing what activate means.

http://codereview.chromium.org/660137/diff/2001/2012
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/660137/diff/2001/2012#newcode206
chrome/common/render_messages_internal.h:206: // selection. If |activate| is
true, it will additionally activate the selected
Nit: 80 cols

http://codereview.chromium.org/660137/diff/2001/2012#newcode207
chrome/common/render_messages_internal.h:207: // node. It is not valid for both
of these parameters to be true.
If it's not valid for both of these params to be true, maybe there should be a
single enum for them like CLEAR_SELECTION, KEEP_SELECTION, and
KEEP_SELECTION_AND_ACTIVATE?

[Finnur, 2010-03-08 22:43:58]

I'd like to see some tests for this functionality, preferably a browser test,
not an interactive test.

I agree with Tony about the enum, it would make the code clearer and remove the
need for commenting two bool params everywhere. Then the nit below can be
ignored.

http://codereview.chromium.org/660137/diff/2001/2007
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/660137/diff/2001/2007#newcode497
chrome/browser/tab_contents/tab_contents.h:497: void StopFinding(bool
clear_selection, bool activate);
nit: Looking just at this API it is not clear what activate means in the context
of StopFinding. Will it highlight the node? Will it click on it? Can you rename
this var?

[Evan Stade, 2010-03-10 01:39:59]

test + enum added

http://codereview.chromium.org/660137/diff/2001/2011
File chrome/browser/cocoa/find_bar_cocoa_controller.mm (right):

http://codereview.chromium.org/660137/diff/2001/2011#newcode138
chrome/browser/cocoa/find_bar_cocoa_controller.mm:138:
tab_contents->StopFinding(true, false);  // true = clear selection on page.
On 2010/03/08 00:24:27, tony wrote:
> The comment here is now confusing.  Maybe just remove the comment?  It might
be
> nice to make enums for this, but I don't feel strongly about it.

Done.

http://codereview.chromium.org/660137/diff/2001/2004
File chrome/browser/find_bar_controller.cc (right):

http://codereview.chromium.org/660137/diff/2001/2004#newcode46
chrome/browser/find_bar_controller.cc:46: tab_contents_->StopFinding(false,
true);
On 2010/03/08 00:24:27, tony wrote:
> Do you mean to pass activate as the second param here?  You're not using
> activate anywhere.

yes, fixed

http://codereview.chromium.org/660137/diff/2001/2010
File chrome/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/660137/diff/2001/2010#newcode207
chrome/browser/renderer_host/render_view_host.h:207: // Cancel a pending find
operation.
On 2010/03/08 00:24:27, tony wrote:
> Nit: Can you keep the comment about clear_selection and add a sentence
> describing what activate means.

I removed it because the commentary about the meaning of the parameters is
repeated several times throughout the code; it seems redundant and likely to get
out of sync.

http://codereview.chromium.org/660137/diff/2001/2007
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/660137/diff/2001/2007#newcode497
chrome/browser/tab_contents/tab_contents.h:497: void StopFinding(bool
clear_selection, bool activate);
On 2010/03/08 22:43:58, Finnur wrote:
> nit: Looking just at this API it is not clear what activate means in the
context
> of StopFinding. Will it highlight the node? Will it click on it? Can you
rename
> this var?

I consider the "clicking" to be an implementation detail, and a regrettable one
at that. I think Activate is the right term for "clicking" a link or "pressing"
a button; I have documented its meaning.

http://codereview.chromium.org/660137/diff/2001/2012
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/660137/diff/2001/2012#newcode206
chrome/common/render_messages_internal.h:206: // selection. If |activate| is
true, it will additionally activate the selected
On 2010/03/08 00:24:27, tony wrote:
> Nit: 80 cols

Done.

http://codereview.chromium.org/660137/diff/2001/2012#newcode207
chrome/common/render_messages_internal.h:207: // node. It is not valid for both
of these parameters to be true.
On 2010/03/08 00:24:27, tony wrote:
> If it's not valid for both of these params to be true, maybe there should be a
> single enum for them like CLEAR_SELECTION, KEEP_SELECTION, and
> KEEP_SELECTION_AND_ACTIVATE?

Done.

[tony, 2010-03-10 05:27:13]

LGTM.  Finnur might want to take another look.

http://codereview.chromium.org/660137/diff/5001/6004
File chrome/browser/find_bar_controller.h (right):

http://codereview.chromium.org/660137/diff/5001/6004#newcode20
chrome/browser/find_bar_controller.h:20: kClearSelection,  // Clear the find
selection.
Huh, I didn't realize that const style was acceptable for enums.  Ok.

[Finnur, 2010-03-10 07:37:41]

http://codereview.chromium.org/660137/diff/5001/6010
File chrome/browser/cocoa/find_bar_cocoa_controller.mm (right):

http://codereview.chromium.org/660137/diff/5001/6010#newcode138
chrome/browser/cocoa/find_bar_cocoa_controller.mm:138:
tab_contents->StopFinding(true, false);
This function signature has changed now.

http://codereview.chromium.org/660137/diff/5001/6010#newcode359
chrome/browser/cocoa/find_bar_cocoa_controller.mm:359:
contents->StopFinding(true, false);
ditto.

http://codereview.chromium.org/660137/diff/5001/6003
File chrome/browser/find_bar_controller.cc (right):

http://codereview.chromium.org/660137/diff/5001/6003#newcode37
chrome/browser/find_bar_controller.cc:37: void
FindBarController::EndFindSession(bool activate) {
Any reason you don't use the enum here too?

http://codereview.chromium.org/660137/diff/5001/6004
File chrome/browser/find_bar_controller.h (right):

http://codereview.chromium.org/660137/diff/5001/6004#newcode20
chrome/browser/find_bar_controller.h:20: kClearSelection,  // Clear the find
selection.
Is it? :)

On 2010/03/10 05:27:14, tony wrote:
> Huh, I didn't realize that const style was acceptable for enums.  Ok.

http://codereview.chromium.org/660137/diff/5001/6004#newcode21
chrome/browser/find_bar_controller.h:21: kKeepSelection,  // Translate the find
selection into a normal selection.
minor nit: KeepSelection should be first enum.

http://codereview.chromium.org/660137/diff/5001/6008
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/660137/diff/5001/6008#newcode400
chrome/browser/renderer_host/render_view_host.cc:400: action.action =
ViewMsg_StopFinding_Params::kClearSelection;
action.action looks weird. Can you instead call this variable params, or
something?

http://codereview.chromium.org/660137/diff/5001/6011
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/660137/diff/5001/6011#newcode207
chrome/common/render_messages_internal.h:207: // selected node. It is not valid
for both of these parameters to be true.
comment out of date.

[Evan Stade, 2010-03-10 16:50:18]

http://codereview.chromium.org/660137/diff/5001/6010
File chrome/browser/cocoa/find_bar_cocoa_controller.mm (right):

http://codereview.chromium.org/660137/diff/5001/6010#newcode138
chrome/browser/cocoa/find_bar_cocoa_controller.mm:138:
tab_contents->StopFinding(true, false);
On 2010/03/10 07:37:41, Finnur wrote:
> This function signature has changed now.

Done.

http://codereview.chromium.org/660137/diff/5001/6003
File chrome/browser/find_bar_controller.cc (right):

http://codereview.chromium.org/660137/diff/5001/6003#newcode37
chrome/browser/find_bar_controller.cc:37: void
FindBarController::EndFindSession(bool activate) {
On 2010/03/10 07:37:41, Finnur wrote:
> Any reason you don't use the enum here too?

because only 2/3 of the enum values would be valid

http://codereview.chromium.org/660137/diff/5001/6004
File chrome/browser/find_bar_controller.h (right):

http://codereview.chromium.org/660137/diff/5001/6004#newcode20
chrome/browser/find_bar_controller.h:20: kClearSelection,  // Clear the find
selection.
On 2010/03/10 07:37:41, Finnur wrote:
> Is it? :)

Enumerators should be named either like constants or like macros: either
kEnumName or ENUM_NAME.

> 
> On 2010/03/10 05:27:14, tony wrote:
> > Huh, I didn't realize that const style was acceptable for enums.  Ok.
> 
>

http://codereview.chromium.org/660137/diff/5001/6004#newcode21
chrome/browser/find_bar_controller.h:21: kKeepSelection,  // Translate the find
selection into a normal selection.
On 2010/03/10 07:37:41, Finnur wrote:
> minor nit: KeepSelection should be first enum. 

because?

http://codereview.chromium.org/660137/diff/5001/6008
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/660137/diff/5001/6008#newcode400
chrome/browser/renderer_host/render_view_host.cc:400: action.action =
ViewMsg_StopFinding_Params::kClearSelection;
On 2010/03/10 07:37:41, Finnur wrote:
> action.action looks weird. Can you instead call this variable params, or
> something?

Done.

http://codereview.chromium.org/660137/diff/5001/6011
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/660137/diff/5001/6011#newcode207
chrome/common/render_messages_internal.h:207: // selected node. It is not valid
for both of these parameters to be true.
On 2010/03/10 07:37:41, Finnur wrote:
> comment out of date.

Done.

[Finnur, 2010-03-10 17:28:11]

http://codereview.chromium.org/660137/diff/5001/6003
File chrome/browser/find_bar_controller.cc (right):

http://codereview.chromium.org/660137/diff/5001/6003#newcode37
chrome/browser/find_bar_controller.cc:37: void
FindBarController::EndFindSession(bool activate) {
Not sure I agree that it is invalid per se, but even so I don't see that as a
big problem (you could DCHECK on it if you feel like that's the right thing to
do).

I would argue that since you already have the enum, the code  would look simpler
that way than having it take a bool, convert it to an enum and then explain at
the call site what passing true/false means.

On 2010/03/10 16:50:19, Evan Stade wrote:
> On 2010/03/10 07:37:41, Finnur wrote:
> > Any reason you don't use the enum here too?
> 
> because only 2/3 of the enum values would be valid

http://codereview.chromium.org/660137/diff/5001/6004
File chrome/browser/find_bar_controller.h (right):

http://codereview.chromium.org/660137/diff/5001/6004#newcode21
chrome/browser/find_bar_controller.h:21: kKeepSelection,  // Translate the find
selection into a normal selection.
It just feels more natural to have it sorted that way (actions ordered by
increased power). In other words, do nothing < clear it < navigate. 

On 2010/03/10 16:50:19, Evan Stade wrote:
> On 2010/03/10 07:37:41, Finnur wrote:
> > minor nit: KeepSelection should be first enum. 
> 
> because?

[Evan Stade, 2010-03-10 17:40:44]

http://codereview.chromium.org/660137/diff/5001/6003
File chrome/browser/find_bar_controller.cc (right):

http://codereview.chromium.org/660137/diff/5001/6003#newcode37
chrome/browser/find_bar_controller.cc:37: void
FindBarController::EndFindSession(bool activate) {
On 2010/03/10 17:28:11, Finnur wrote:
> Not sure I agree that it is invalid per se, but even so I don't see that as a
> big problem (you could DCHECK on it if you feel like that's the right thing to
> do).
> 
> I would argue that since you already have the enum, the code  would look
simpler
> that way than having it take a bool, convert it to an enum and then explain at
> the call site what passing true/false means.
> 
> On 2010/03/10 16:50:19, Evan Stade wrote:
> > On 2010/03/10 07:37:41, Finnur wrote:
> > > Any reason you don't use the enum here too?
> > 
> > because only 2/3 of the enum values would be valid
> 
> 

I believe in making it as easy as possible for calling code to do the right
thing. But I will change it if you want (let me know :).

http://codereview.chromium.org/660137/diff/5001/6004
File chrome/browser/find_bar_controller.h (right):

http://codereview.chromium.org/660137/diff/5001/6004#newcode21
chrome/browser/find_bar_controller.h:21: kKeepSelection,  // Translate the find
selection into a normal selection.
On 2010/03/10 17:28:11, Finnur wrote:
> It just feels more natural to have it sorted that way (actions ordered by
> increased power). In other words, do nothing < clear it < navigate. 
> 
> On 2010/03/10 16:50:19, Evan Stade wrote:
> > On 2010/03/10 07:37:41, Finnur wrote:
> > > minor nit: KeepSelection should be first enum. 
> > 
> > because?
> 
> 

Done.

[Finnur, 2010-03-10 17:43:43]

http://codereview.chromium.org/660137/diff/5001/6003
File chrome/browser/find_bar_controller.cc (right):

http://codereview.chromium.org/660137/diff/5001/6003#newcode37
chrome/browser/find_bar_controller.cc:37: void
FindBarController::EndFindSession(bool activate) {
Yes, thank you. Appreciate it.

[Evan Stade, 2010-03-10 18:04:06]

updated

[Finnur, 2010-03-10 18:11:06]

LGTM - thanks!