Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(7)

Issue 280005: Let cmd-` switch windows again. (Closed)

Created:
11 years, 2 months ago by Nico
Modified:
9 years, 7 months ago
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+25 lines, -0 lines) Patch
M chrome/browser/tab_contents/tab_contents_view_mac.mm View 3 chunks +25 lines, -0 lines 4 comments Download

Messages

Total messages: 6 (0 generated)
Nico
Not the most beautiful thing in the world, but as long this is the only ...
11 years, 2 months ago (2009-10-15 06:39:30 UTC) #1
James Su
Hi, I only have a Macbook Pro with US keyboard, not sure if it's ok ...
11 years, 2 months ago (2009-10-15 08:00:49 UTC) #2
Avi (use Gerrit)
I'm a little bit better about it than James, but I'm wondering if the better ...
11 years, 2 months ago (2009-10-15 13:35:52 UTC) #3
Nico
James: I tested with an US keyboard and a different layout. For that case, this ...
11 years, 2 months ago (2009-10-15 15:40:00 UTC) #4
Avi (use Gerrit)
LG
11 years, 2 months ago (2009-10-15 15:46:52 UTC) #5
James Su
11 years, 2 months ago (2009-10-16 00:18:00 UTC) #6
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).

Powered by Google App Engine
This is Rietveld 408576698