Fix cmd-up/cmd-down.

Fix cmd-up/cmd-down.

The approach is to special-case moveToBeginningOfDocument and moveToEndOfDocument in WebFrameImpl::executeCommand(). With this (and the cocoa keyboard bindings patch being landed), the special-case code in WebViewImpl::ScrollViewWithKeyboard() can be removed.

Also add a test for cmd-up/down.

Change chrome.gyp so that it supports osx-only unit tests (_unittest_mac.mm).

Move OnPrintPageAsBitmap to the other printing tests.


BUG=22585
TEST=Go to a page with both text box and scrollbar (e.g. http://en.wikipedia.org ). Pressing cmd-down should scroll to end of page, cmd-up back to start. Clicking the text box and trying some emacs shortcuts should work (ctrl-a jumps to start of line, cmd-h acts as backspace, etc).

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

[Nico, 2009-09-28 15:18:57]

darin: I'd appreciate a quick look at webview_impl.h and
WebFrameImpl::executeCommand(const WebString&, const WebString&).

hbono, suzhe: Please review everything.

[darin (slow to review), 2009-09-28 16:39:11]

LG

http://codereview.chromium.org/254002/diff/5002/4011
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/254002/diff/5002/4011#newcode1535
Line 1535: else
no need for 'else' after 'break'

http://codereview.chromium.org/254002/diff/5002/4017
File webkit/glue/webframe_impl.cc (right):

http://codereview.chromium.org/254002/diff/5002/4017#newcode1019
Line 1019: EqualsASCII(name, "moveToBeginningOfDocument")) {
instead of EqualsASCII, please create a local variable at the top of this file
to hold the result of webkit_glue::WebStringToString(name).  then, you can just
use operator== with the resultant WebCore::String type, and you can finally just
pass that local variable as the parameter to Editor::command.

[Nico, 2009-09-28 22:12:27]

http://codereview.chromium.org/254002/diff/5002/4011
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/254002/diff/5002/4011#newcode1535
Line 1535: else
On 2009/09/28 16:39:11, darin wrote:
> no need for 'else' after 'break'

Done.

http://codereview.chromium.org/254002/diff/5002/4017
File webkit/glue/webframe_impl.cc (right):

http://codereview.chromium.org/254002/diff/5002/4017#newcode1019
Line 1019: EqualsASCII(name, "moveToBeginningOfDocument")) {
On 2009/09/28 16:39:11, darin wrote:
> instead of EqualsASCII, please create a local variable at the top of this file
> to hold the result of webkit_glue::WebStringToString(name).  then, you can
just
> use operator== with the resultant WebCore::String type, and you can finally
just
> pass that local variable as the parameter to Editor::command.

Done (with s/file/function/)

[James Su, 2009-09-29 00:55:51]

LGTM.

Just wondering if there are other edit commands need such special treatment?

James Su

http://codereview.chromium.org/254002/diff/4/1026
File webkit/glue/webview_impl.h (right):

http://codereview.chromium.org/254002/diff/4/1026#newcode259
Line 259: // Tries to scroll a frame or any parent of a fram. Returns true if
the view
s/fram/frame/

[Nico, 2009-09-29 03:52:32]

> Just wondering if there are other edit commands need such special treatment?

I don't know. It would theoretically be good to have tests for every one of
them, but that's a lot of work. For now, I guess we'll add tests when we find
problems :-/

hbono, if you don't mind I will submit this, so that it makes it into this
week's dev release. If you have comments, I will address them in a follow-up CL.

[Nico, 2009-09-29 03:52:46]

http://codereview.chromium.org/254002/diff/4/1026
File webkit/glue/webview_impl.h (right):

http://codereview.chromium.org/254002/diff/4/1026#newcode259
Line 259: // Tries to scroll a frame or any parent of a fram. Returns true if
the view
On 2009/09/29 00:55:51, James Su wrote:
> s/fram/frame/

Done.

[jeremy, 2009-09-29 18:12:56]

http://codereview.chromium.org/254002/diff/6/8
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/254002/diff/6/8#newcode1519
Line 1519: // not execute the rest.
Are we sure this isn't possible on OSX as well?
According to http://www.erasetotheleft.com/post/mac-os-x-key-bindings/ you can
bind a key to multiple edit commands on OS X.

http://codereview.chromium.org/254002/diff/6/11
File chrome/renderer/render_view_unittest_mac.mm (right):

http://codereview.chromium.org/254002/diff/6/11#newcode15
Line 15: 
extra newline.

http://codereview.chromium.org/254002/diff/6/11#newcode18
Line 18: case kVK_UpArrow: uniChar = NSUpArrowFunctionKey; break;
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Loops_...:

switch () {
  case bla:
     foo;
    break;

http://codereview.chromium.org/254002/diff/6/11#newcode39
Line 39: 
extra newline

http://codereview.chromium.org/254002/diff/6/11#newcode94
Line 94: "40,false,false,true,false\n1936\np1\n\np2";
Could you provide a helper function that does the equality check rather than
doing an EXPECT_EQ with this raw string, that would make this easier to modify
and understand later on.

http://codereview.chromium.org/254002/diff/6/13
File chrome/test/render_view_test.h (right):

http://codereview.chromium.org/254002/diff/6/13#newcode14
Line 14: #include "chrome/common/native_web_keyboard_event.h"
alphabetize include order.

http://codereview.chromium.org/254002/diff/6/14
File webkit/glue/webframe_impl.cc (right):

http://codereview.chromium.org/254002/diff/6/14#newcode1018
Line 1018: // for editable nodes.
hbono had quite alot to say about this, could you elaborate on this comment if
there's anything more to say here?

[hbono1, 2009-09-30 03:05:04]

thakis,

Thank you for your change and sorry for my slow response.
Since I have been taking a vacation this week only with an Android
phone, it is very hard for me to review this change now. So, I don't
mind you land this change at all. I will check this change next week
and write a follow-up change if I need add my comments.

Regards,

Hironori Bono

On Tue, Sep 29, 2009 at 12:52 PM,  <thakis@chromium.org> wrote:
>> Just wondering if there are other edit commands need such special
>> treatment?
>
> I don't know. It would theoretically be good to have tests for every one of
> them, but that's a lot of work. For now, I guess we'll add tests when we
> find
> problems :-/
>
> hbono, if you don't mind I will submit this, so that it makes it into this
> week's dev release. If you have comments, I will address them in a follow-up
> CL.
>
> http://codereview.chromium.org/254002
>

[Nico, 2009-09-30 03:51:34]

Sorry about all the style issues. I've uploaded a followup-CL to
http://codereview.chromium.org/244044 .

http://codereview.chromium.org/254002/diff/6/8
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/254002/diff/6/8#newcode1519
Line 1519: // not execute the rest.
On 2009/09/29 18:12:56, jeremy wrote:
> Are we sure this isn't possible on OSX as well?
> According to http://www.erasetotheleft.com/post/mac-os-x-key-bindings/ you can
> bind a key to multiple edit commands on OS X.

Oh, this wasn't supposed to mean "only in gtk" but "for example in gtk". I guess
being explicit doesn't hurt. Done.

http://codereview.chromium.org/254002/diff/6/11
File chrome/renderer/render_view_unittest_mac.mm (right):

http://codereview.chromium.org/254002/diff/6/11#newcode15
Line 15: 
On 2009/09/29 18:12:56, jeremy wrote:
> extra newline.

Done.

http://codereview.chromium.org/254002/diff/6/11#newcode18
Line 18: case kVK_UpArrow: uniChar = NSUpArrowFunctionKey; break;
On 2009/09/29 18:12:56, jeremy wrote:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Loops_...:
> 
> switch () {
>   case bla:
>      foo;
>     break;

Surely the styleguide wants to say
switch() {
  case bla:
    foo;
    break;
}

? (i.e. the "4 indent" is relative to the "switch")

Anyway, indented the "case"s, but kept them as one line. Is that ok?

http://codereview.chromium.org/254002/diff/6/11#newcode39
Line 39: 
On 2009/09/29 18:12:56, jeremy wrote:
> extra newline

Done.

http://codereview.chromium.org/254002/diff/6/11#newcode94
Line 94: "40,false,false,true,false\n1936\np1\n\np2";
On 2009/09/29 18:12:56, jeremy wrote:
> Could you provide a helper function that does the equality check rather than
> doing an EXPECT_EQ with this raw string, that would make this easier to modify
> and understand later on.

Not easily. The helper function would have to go into render_view_test.h, which
is used on win and linux as well, and the most general function I can come up
for this isn't useful for other test either (the function would look like
ExpectPageText(keyCode, isShiftdown, isCtrlDown, isMetaDown, isAltDown,
scrollOffset, trailingText).

http://codereview.chromium.org/254002/diff/6/13
File chrome/test/render_view_test.h (right):

http://codereview.chromium.org/254002/diff/6/13#newcode14
Line 14: #include "chrome/common/native_web_keyboard_event.h"
On 2009/09/29 18:12:56, jeremy wrote:
> alphabetize include order.

Done.

http://codereview.chromium.org/254002/diff/6/14
File webkit/glue/webframe_impl.cc (right):

http://codereview.chromium.org/254002/diff/6/14#newcode1018
Line 1018: // for editable nodes.
On 2009/09/29 18:12:56, jeremy wrote:
> hbono had quite alot to say about this, could you elaborate on this comment if
> there's anything more to say here?

Not sure what you're after…he suggested that we should disable edit commands for
non-editable nodes in general, but that breaks e.g. selectAll as noted on the
bug, so I came up with this approach. Do you want me to add something to the
comment (what?) or…?