Another attempt at fixing cmd-left/right.

Another attempt at fixing cmd-left/right.

This basically relands http://codereview.chromium.org/347022 . With bugs 26735 and 25205 fixed, this should now not regress anything.

BUG=25856
TEST=Go to en.wikipedia.org , make the window very narrow so that there's a horizontal scrollbar. cmd-left/right should go back/forward instead of scrolling horizontally.

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

[Nico, 2009-11-07 00:41:55]

…because you're the only one who might review this today :-)

[viettrungluu, 2009-11-07 01:30:57]

Just picked a few nits (in the comments), and asked one real question....

http://codereview.chromium.org/378019/diff/1/2
File webkit/api/src/EditorClientImpl.cpp (right):

http://codereview.chromium.org/378019/diff/1/2#newcode365
Line 365: // http://crbug.com/25856 and the bugs linked from there for why.
Could you please just briefly explain why?

http://codereview.chromium.org/378019/diff/1/3
File webkit/api/src/mac/WebInputEventFactory.mm (right):

http://codereview.chromium.org/378019/diff/1/3#newcode922
Line 922: // Windows and Linux set that if alt is down. WebKit looks at this to
decide
What are "that" and "this"?

http://codereview.chromium.org/378019/diff/1/3#newcode924
Line 924: // by webkit to scroll the current page, because we want to get that
key
Is it "WebKit" or "webkit"? ;-)

http://codereview.chromium.org/378019/diff/1/3#newcode927
Line 927: if (result.modifiers & WebInputEvent::MetaKey)
Meta == Cmd, I guess? Moreover, do we necessarily want all Cmd-key combinations
back? (I assume so, but could it have any bad consequences?)

http://codereview.chromium.org/378019/diff/1/3#newcode950
Line 950: // Windows and Linux set that if alt is down. WebKit looks at this to
decide
Ditto. (Or perhaps you could just refer to the comment in the previous method to
avoid this duplication.)

[Nico, 2009-11-07 01:43:54]

http://codereview.chromium.org/378019/diff/1/2
File webkit/api/src/EditorClientImpl.cpp (right):

http://codereview.chromium.org/378019/diff/1/2#newcode365
Line 365: // http://crbug.com/25856 and the bugs linked from there for why.
On 2009/11/07 01:30:57, viettrungluu wrote:
> Could you please just briefly explain why?

Done.

http://codereview.chromium.org/378019/diff/1/3
File webkit/api/src/mac/WebInputEventFactory.mm (right):

http://codereview.chromium.org/378019/diff/1/3#newcode922
Line 922: // Windows and Linux set that if alt is down. WebKit looks at this to
decide
On 2009/11/07 01:30:57, viettrungluu wrote:
> What are "that" and "this"?

Done.

http://codereview.chromium.org/378019/diff/1/3#newcode924
Line 924: // by webkit to scroll the current page, because we want to get that
key
On 2009/11/07 01:30:57, viettrungluu wrote:
> Is it "WebKit" or "webkit"? ;-)

Done.

http://codereview.chromium.org/378019/diff/1/3#newcode927
Line 927: if (result.modifiers & WebInputEvent::MetaKey)
On 2009/11/07 01:30:57, viettrungluu wrote:
> Meta == Cmd, I guess? Moreover, do we necessarily want all Cmd-key
combinations
> back? (I assume so, but could it have any bad consequences?)

Yes, Meta == Cmd. Yes, it could have bad consequences, see the last time I
landed this :-) Hopefully it works better this time. I tested all the Meta key
shortcuts defined in EditClientImpl.cpp, at least these still seem to work.

[viettrungluu, 2009-11-07 02:21:47]

On 2009/11/07 01:43:54, Nico wrote:
> Yes, Meta == Cmd. Yes, it could have bad consequences, see the last time I
> landed this :-) Hopefully it works better this time. I tested all the Meta key
> shortcuts defined in EditClientImpl.cpp, at least these still seem to work.

Hrm, that makes me nervous, but obviously others have okayed it before. ;-) So
I'm willing to give it a go (launch and iterate, right?). LGTM.