[Mac] Make the pageup, Shift-pageup, and Option-Shift-pageup keys scroll the ...

[Mac] Make the pageup, Shift-pageup, and Option-Shift-pageup keys scroll the underlying webpage when using the findbar.

BUG=http://crbug.com/17421
TEST=Try pressing pageup with focus in the findbar.

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

[rohitrao (ping after 24h), 2009-09-14 18:54:47]

The listed six selectors all scroll the webpage with the findbar closed, so I
figured I would support all of them with the findbar open.

Scott, you get the review because you filed the bug :)

[Scott Hess - ex-Googler, 2009-09-14 20:39:56]

LGTM with the indicated line moved.  Other comment just random thought.

http://codereview.chromium.org/204006/diff/1/2
File chrome/browser/cocoa/find_bar_cocoa_controller.mm (right):

http://codereview.chromium.org/204006/diff/1/2#newcode126
Line 126: RenderViewHost* render_view_host = contents->render_view_host();
Move this down by use.

http://codereview.chromium.org/204006/diff/1/2#newcode133
Line 133: render_view_host->ForwardKeyboardEvent(NativeWebKeyboardEvent(event));
Would it make sense to call [contents->GetNativeView() keyEvent:event]?  Or is
that too crazy?  Hmm.  Probably too crazy.

[rohitrao (ping after 24h), 2009-09-14 21:07:52]

http://codereview.chromium.org/204006/diff/1/2
File chrome/browser/cocoa/find_bar_cocoa_controller.mm (right):

http://codereview.chromium.org/204006/diff/1/2#newcode126
Line 126: RenderViewHost* render_view_host = contents->render_view_host();
On 2009/09/14 20:39:56, shess wrote:
> Move this down by use.

Done.

http://codereview.chromium.org/204006/diff/1/2#newcode133
Line 133: render_view_host->ForwardKeyboardEvent(NativeWebKeyboardEvent(event));
On 2009/09/14 20:39:56, shess wrote:
> Would it make sense to call [contents->GetNativeView() keyEvent:event]?  Or is
> that too crazy?  Hmm.  Probably too crazy.

[contents->GetContentNativeView() keyUp:event] works, but it ends up being more
code because I have to split out keyUp/keyDown.  Leaving this as-is for now.

[Scott Hess - ex-Googler, 2009-09-14 21:11:35]

I actually mis-spoke on the -keyUp:  -keyEvent: would do it :-).

In fact, you might want to follow-up and look at that method, just in
case there are interesting things it's doing that your code should be.
 I don't see anything offhand, but then again I don't claim to
understand that code!

-scott


On Mon, Sep 14, 2009 at 2:07 PM,  <rohitrao@chromium.org> wrote:
>
> http://codereview.chromium.org/204006/diff/1/2
> File chrome/browser/cocoa/find_bar_cocoa_controller.mm (right):
>
> http://codereview.chromium.org/204006/diff/1/2#newcode126
> Line 126: RenderViewHost* render_view_host =3D
> contents->render_view_host();
> On 2009/09/14 20:39:56, shess wrote:
>>
>> Move this down by use.
>
> Done.
>
> http://codereview.chromium.org/204006/diff/1/2#newcode133
> Line 133:
> render_view_host->ForwardKeyboardEvent(NativeWebKeyboardEvent(event));
> On 2009/09/14 20:39:56, shess wrote:
>>
>> Would it make sense to call [contents->GetNativeView()
>
> keyEvent:event]? =A0Or is
>>
>> that too crazy? =A0Hmm. =A0Probably too crazy.
>
> [contents->GetContentNativeView() keyUp:event] works, but it ends up
> being more code because I have to split out keyUp/keyDown. =A0Leaving thi=
s
> as-is for now.
>
> http://codereview.chromium.org/204006
>

[Scott Hess - ex-Googler, 2009-09-14 21:14:07]

BTW, this comment is not intended to negate the earlier LGTM.

On Mon, Sep 14, 2009 at 2:11 PM, Scott Hess <shess@chromium.org> wrote:
> I actually mis-spoke on the -keyUp: =A0-keyEvent: would do it :-).
>
> In fact, you might want to follow-up and look at that method, just in
> case there are interesting things it's doing that your code should be.
> =A0I don't see anything offhand, but then again I don't claim to
> understand that code!
>
> -scott
>
>
> On Mon, Sep 14, 2009 at 2:07 PM, =A0<rohitrao@chromium.org> wrote:
>>
>> http://codereview.chromium.org/204006/diff/1/2
>> File chrome/browser/cocoa/find_bar_cocoa_controller.mm (right):
>>
>> http://codereview.chromium.org/204006/diff/1/2#newcode126
>> Line 126: RenderViewHost* render_view_host =3D
>> contents->render_view_host();
>> On 2009/09/14 20:39:56, shess wrote:
>>>
>>> Move this down by use.
>>
>> Done.
>>
>> http://codereview.chromium.org/204006/diff/1/2#newcode133
>> Line 133:
>> render_view_host->ForwardKeyboardEvent(NativeWebKeyboardEvent(event));
>> On 2009/09/14 20:39:56, shess wrote:
>>>
>>> Would it make sense to call [contents->GetNativeView()
>>
>> keyEvent:event]? =A0Or is
>>>
>>> that too crazy? =A0Hmm. =A0Probably too crazy.
>>
>> [contents->GetContentNativeView() keyUp:event] works, but it ends up
>> being more code because I have to split out keyUp/keyDown. =A0Leaving th=
is
>> as-is for now.
>>
>> http://codereview.chromium.org/204006
>>
>