Let cmd-` switch windows again.

Let cmd-` switch windows again.

BUG=24817
TEST=Open three windows. Focus web contents. Cmd-` should cycle windows Cmd-shift-` should cycle in the other direction.

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

[Nico, 2009-10-15 06:39:30]

Not the most beautiful thing in the world, but as long this is the only key for
which this is necessary, it's ok.

James: I'm actually not sure if it's better to check the keycode or the
character on an international keyboard. Do you have a mac with a non-english
keyboard? Can you check if this works as expected with your keyboard? Could you
also try replacing

      [event keyCode] == kVK_ANSI_Grave &&

with

      [[event characters] isEqualToString:@"`"] &&

and see if it changes things?

[James Su, 2009-10-15 08:00:49]

Hi,
  I only have a Macbook Pro with US keyboard, not sure if it's ok to test it
with other keyboard layout settings. I'll test it when I back to home.
  However I don't feel so good with this approach, we probably need reconsider
the solution we took for cmd keys.

Regards
James Su

http://codereview.chromium.org/280005/diff/1/2
File chrome/browser/tab_contents/tab_contents_view_mac.mm (right):

http://codereview.chromium.org/280005/diff/1/2#newcode265
Line 265: - (void)_cycleWindowsReversed:(BOOL)reversed;
Is it ok for us to depend on a private method? It makes me a little nervous. And
what's the difference between _cycleWindows, _cycleWindowsBackwards and
_cycleWindowsReversed?

http://codereview.chromium.org/280005/diff/1/2#newcode315
Line 315: // ` key, so do this by keycode instead of |event characters|.
Is Cmd-` the only key handled by |NSApp sendEvent| but not in the menu? I feel
that the right approach is to feed the key event back to NSApp, so that we don't
need to care about how to handle it.

[Avi (use Gerrit), 2009-10-15 13:35:52]

I'm a little bit better about it than James, but I'm wondering if the better
place for this wouldn't be CrApplication rather than the TCV.

[Nico, 2009-10-15 15:40:00]

James: I tested with an US keyboard and a different layout. For that case, this
seems to be right. I understand your reservations about this patch, I feel
similar. If one more case like this turns up, I'll change this to the
|sendEvent| approach, but having one exception seems to be still in line for me.
Ok?

Avi: "this" being? The code has to be invoked from TVCMac, so that it happens
after the key comes back from the renderer. I could add some new method to CrApp
that does dispatching to the menu and this new thing, but this way the code is
more localized (which also means it's easier to revert if we have to switch to
the other approach).

http://codereview.chromium.org/280005/diff/1/2
File chrome/browser/tab_contents/tab_contents_view_mac.mm (right):

http://codereview.chromium.org/280005/diff/1/2#newcode265
Line 265: - (void)_cycleWindowsReversed:(BOOL)reversed;
On 2009/10/15 08:00:49, James Su wrote:
> Is it ok for us to depend on a private method? It makes me a little nervous.
And
> what's the difference between _cycleWindows, _cycleWindowsBackwards and
> _cycleWindowsReversed?

I think it's ok for often-used stuff as long as we check that the function
exists before calling it – that way we don't crash but only lose some
functionality if it goes away. Which we will notice, because when something
that's used often stops working, someone will notice. We do this in several
places already.

WebKit uses this function too (
https://svn.webkit.org/browser/trunk/WebKit/mac/DefaultDelegates/WebDefaultUI...
) and has for years, so this going away all of a sudden is unlikely.

_cycleWindowsReversed: receives a bool argument, while _cycleWindows: and
_cycleWindowsBackwards: seem to receive an (id)sender, so that the latter two
can be used as actions.

http://codereview.chromium.org/280005/diff/1/2#newcode315
Line 315: // ` key, so do this by keycode instead of |event characters|.
On 2009/10/15 08:00:49, James Su wrote:
> Is Cmd-` the only key handled by |NSApp sendEvent| but not in the menu? I feel
> that the right approach is to feed the key event back to NSApp, so that we
don't
> need to care about how to handle it.

Experimental evidence suggests that it really is the only key, but I don't know.
(By "experimental evidence" I mean that it's the only key we received bugs for,
and it's also the only key that needed special handling in other projects I've
seen).

[Avi (use Gerrit), 2009-10-15 15:46:52]

LG

[James Su, 2009-10-16 00:18:00]

Thanks for your explanation. I'm ok with it.

James Su

On 2009/10/15 15:40:00, Nico wrote:
> James: I tested with an US keyboard and a different layout. For that case,
this
> seems to be right. I understand your reservations about this patch, I feel
> similar. If one more case like this turns up, I'll change this to the
> |sendEvent| approach, but having one exception seems to be still in line for
me.
> Ok?
> 
> Avi: "this" being? The code has to be invoked from TVCMac, so that it happens
> after the key comes back from the renderer. I could add some new method to
CrApp
> that does dispatching to the menu and this new thing, but this way the code is
> more localized (which also means it's easier to revert if we have to switch to
> the other approach).
> 
> http://codereview.chromium.org/280005/diff/1/2
> File chrome/browser/tab_contents/tab_contents_view_mac.mm (right):
> 
> http://codereview.chromium.org/280005/diff/1/2#newcode265
> Line 265: - (void)_cycleWindowsReversed:(BOOL)reversed;
> On 2009/10/15 08:00:49, James Su wrote:
> > Is it ok for us to depend on a private method? It makes me a little nervous.
> And
> > what's the difference between _cycleWindows, _cycleWindowsBackwards and
> > _cycleWindowsReversed?
> 
> I think it's ok for often-used stuff as long as we check that the function
> exists before calling it – that way we don't crash but only lose some
> functionality if it goes away. Which we will notice, because when something
> that's used often stops working, someone will notice. We do this in several
> places already.
> 
> WebKit uses this function too (
>
https://svn.webkit.org/browser/trunk/WebKit/mac/DefaultDelegates/WebDefaultUI...
> ) and has for years, so this going away all of a sudden is unlikely.
> 
> _cycleWindowsReversed: receives a bool argument, while _cycleWindows: and
> _cycleWindowsBackwards: seem to receive an (id)sender, so that the latter two
> can be used as actions.
> 
> http://codereview.chromium.org/280005/diff/1/2#newcode315
> Line 315: // ` key, so do this by keycode instead of |event characters|.
> On 2009/10/15 08:00:49, James Su wrote:
> > Is Cmd-` the only key handled by |NSApp sendEvent| but not in the menu? I
feel
> > that the right approach is to feed the key event back to NSApp, so that we
> don't
> > need to care about how to handle it.
> 
> Experimental evidence suggests that it really is the only key, but I don't
know.
> (By "experimental evidence" I mean that it's the only key we received bugs
for,
> and it's also the only key that needed special handling in other projects I've
> seen).