Make window cycling work even if you change it to something else than cmd-` in sysprefs.

Make window cycling work even if you change it to something else than cmd-` in sysprefs.

Instead of just dispatching to the menu after a key comes back from the renderer, do a complete re-dispatch to NSApp (so that the event gets to the menu and cmd-` handlers) but then ignore it when it comes back to the web (because we already sent this event to the renderer once).

BUG=24817
TEST=Open sysprefs, change keyboard shortcut for "Move focus to next window in active application" to e.g. cmd-\. Open two chrome windows, focus the web, hit cmd-\. It should switch windows. All other keyboard shortcuts should still work (test that ctrl-tab works when web has focus, test backspace when text field is focussed in web, when background is focussed in web, when IME is active, test hitting cmd-1/2 when omnibox or web have focus, hit cmd-left when omnibox, textbox in web, background in web has focus, test that cmd-f in docs still opens doc's find interface)

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

[Avi (use Gerrit), 2009-10-19 20:12:10]

The growing complexity of this code makes me sadder and sadder. Overall LG with
comments.

http://codereview.chromium.org/303002/diff/3001/2008
File chrome/browser/chrome_application_mac.mm (right):

http://codereview.chromium.org/303002/diff/3001/2008#newcode195
Line 195: if ([static_cast<ChromeEventProcessingWindow*>([self keyWindow])
why the if? if [self keyWindow] is nil, then we've already messaged it on the
previous line and gotten false.

http://codereview.chromium.org/303002/diff/3001/2013
File chrome/browser/tab_contents/tab_contents_view_mac.mm (right):

http://codereview.chromium.org/303002/diff/3001/2013#newcode320
Line 320: // If this tab is no longer active, it's window will be |nil|. In that
case,
extra space at the end of this line? BTW, fix "it's" to "its".

[Nico, 2009-10-19 20:22:57]

I'm sad about the complexity, too.  We tried the less complex way first, but
Cocoa is usually a pain when you try to do things that it's not designed for
(which we very much do here).

http://codereview.chromium.org/303002/diff/3001/2008
File chrome/browser/chrome_application_mac.mm (right):

http://codereview.chromium.org/303002/diff/3001/2008#newcode195
Line 195: if ([static_cast<ChromeEventProcessingWindow*>([self keyWindow])
On 2009/10/19 20:12:10, Avi wrote:
> why the if? if [self keyWindow] is nil, then we've already messaged it on the
> previous line and gotten false.

The if is supposed to check the result of shortcircuitEvent:, which returns NO
if this event should not be shortcircuited (e.g. during redispatch).

http://codereview.chromium.org/303002/diff/3001/2013
File chrome/browser/tab_contents/tab_contents_view_mac.mm (right):

http://codereview.chromium.org/303002/diff/3001/2013#newcode320
Line 320: // If this tab is no longer active, it's window will be |nil|. In that
case,
On 2009/10/19 20:12:10, Avi wrote:
> extra space at the end of this line? BTW, fix "it's" to "its".

Done.

[Nico, 2009-10-19 21:15:49]

I'm going to submit this with Avi's LG. Jeremy & James, feel free to leave
comments, I'll address them in a followup CL.

[James Su, 2009-10-20 00:40:19]

LGTM. Thanks for your great work.

James Su

On 2009/10/19 21:15:49, Nico wrote:
> I'm going to submit this with Avi's LG. Jeremy & James, feel free to leave
> comments, I'll address them in a followup CL.

[pink (ping after 24hrs), 2009-10-20 16:14:43]

http://codereview.chromium.org/303002/diff/5002/6008
File chrome/browser/tab_contents/tab_contents_view_mac.mm (right):

http://codereview.chromium.org/303002/diff/5002/6008#newcode12
Line 12: #import "chrome/browser/chrome_application_mac.h"
i don't see why this include is needed. Why does the TCV need to know about the
application?

[Nico, 2009-10-20 16:32:24]

http://codereview.chromium.org/303002/diff/5002/6008
File chrome/browser/tab_contents/tab_contents_view_mac.mm (right):

http://codereview.chromium.org/303002/diff/5002/6008#newcode12
Line 12: #import "chrome/browser/chrome_application_mac.h"
On 2009/10/20 16:14:44, pink wrote:
> i don't see why this include is needed. Why does the TCV need to know about
the
> application?

Historical reasons. http://codereview.chromium.org/308002 . Thanks.

[jeremy, 2009-10-21 12:36:14]

Nico: Thanks for fixing this!  Regarding Avi's comment, I think we're to the
point where we absolutely need a wiki article outlining this code and
pointing to relevant bugs for justification of why things work the way they
do.

As a general remark I think that unit testing in this area is pretty
critical since the behavior is getting more and more subtle as time
continues.

On Tue, Oct 20, 2009 at 6:32 PM, <thakis@chromium.org> wrote:

>
> http://codereview.chromium.org/303002/diff/5002/6008
> File chrome/browser/tab_contents/tab_contents_view_mac.mm (right):
>
> http://codereview.chromium.org/303002/diff/5002/6008#newcode12
> Line 12: #import "chrome/browser/chrome_application_mac.h"
> On 2009/10/20 16:14:44, pink wrote:
>
>> i don't see why this include is needed. Why does the TCV need to know
>>
> about the
>
>> application?
>>
>
> Historical reasons. http://codereview.chromium.org/308002 . Thanks.
>
>
> http://codereview.chromium.org/303002
>

[Avi (use Gerrit), 2009-10-21 15:05:19]

On 2009/10/21 12:36:14, jeremy wrote:
> we absolutely need a wiki article

Or a block comment in the code (that's my style; see what I've done in the mouse
event file). Either way, but get it down in words before it falls away from your
mind.

[Nico, 2009-10-21 15:09:31]

Ok, I'll write a wiki page (and tests). Where would I put it? On the
"design documents" section at dev.chromium.org?

On Wed, Oct 21, 2009 at 8:05 AM,  <avi@chromium.org> wrote:
> On 2009/10/21 12:36:14, jeremy wrote:
>>
>> we absolutely need a wiki article
>
> Or a block comment in the code (that's my style; see what I've done in the
> mouse
> event file). Either way, but get it down in words before it falls away from
> your
> mind.
>
> http://codereview.chromium.org/303002
>