Activate Chromium after opening a new window.

The application is activated after opening a new window from the Dock menu, so you can type into the Omnibar right away (picking something from the Dock menu doesn't seem to activate the application).

See also: http://codereview.chromium.org/196103

BUG=21175
TEST=Try opening new windows from the dock menu when either Chrome or another application is active.

Closed by mark on 2009-10-24, landed by sgk at r26819 on 2009-09-22 for Vedran under http://codereview.chromium.org/207054.

[vedran.sajatovic, 2009-09-14 22:35:57]

Fixed some gclient mess.. patch 3 is the right one ;-)

[Nico, 2009-09-14 23:09:19]

Hi Vedran,

cool that you want to help out. I see that you signed the CLA already, which is
good too.

It's an unlucky coincidence that sgk worked on this too, there's some wasted
effort here :-/

Anyway, putting the menu into the xib instead of in code makes sense to me. The
other changes kind of make sense to me, but they might also break something. I'm
not familiar with this code, so I'm including some of the people from sgk's CL
to review this.

Nico

http://codereview.chromium.org/196113/diff/2004/21
File chrome/browser/app_controller_mac_unittest.mm (left):

http://codereview.chromium.org/196113/diff/2004/21#oldcode15
Line 15: AppController* ac = [[[AppController alloc] init] autorelease];
Could you keep this text and instead use the menu loaded from the xib?

[sgk, 2009-09-15 13:48:48]

I had actually started implementing this in MainMenu.xib, but it was suggested
that there might be vague, undefined plans to generate dynamic dock menu content
in the future (history e.g.), in which case AIUI it might end up back in code
anyway.  *shrug*  I'm okay either way.  If the Mac team consensus is that this
is a preferable solution, even for the short term, cool by me.

[sgk, 2009-09-15 13:48:49]

I had actually started implementing this in MainMenu.xib, but it was suggested
that there might be vague, undefined plans to generate dynamic dock menu content
in the future (history e.g.), in which case AIUI it might end up back in code
anyway.  *shrug*  I'm okay either way.  If the Mac team consensus is that this
is a preferable solution, even for the short term, cool by me.

[sgk, 2009-09-15 13:48:49]

I had actually started implementing this in MainMenu.xib, but it was suggested
that there might be vague, undefined plans to generate dynamic dock menu content
in the future (history e.g.), in which case AIUI it might end up back in code
anyway.  *shrug*  I'm okay either way.  If the Mac team consensus is that this
is a preferable solution, even for the short term, cool by me.

[vedran.sajatovic, 2009-09-15 21:47:13]

Hi,

I was of course not familiar with those future plans - in that case, I'd stick
to the menu implementation in code.

However, two issues remain: new windows created by the Dock menu (regardless of
how the menu is implemented) are not activated (you have to click on the Omnibox
before you can start typing into it), and the "new incognito window" Dock menu
item doesn't always open a new window, but prefers to add a tab to an existing
incognito window. The updated patch set addresses those issues. I'd appreciate
your comments.

Regards, Vedran

[John Grabowski, 2009-09-16 20:14:02]

Hi Vedran.  Thanks for contributing!

Not clear where we will go for the dock menu, but it would be a shame to back
out Stephen's changes... just to add them back in again real soon.  I suspect
we'll at least experiment here.

I like what you've found but I think it needs a little refinement before
landing.

http://codereview.chromium.org/196113/diff/8004/4004
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/196113/diff/8004/4004#newcode472
Line 472: Browser::OpenEmptyWindow(defaultProfile->GetOffTheRecordProfile());
I don't believe this change makes any difference.
If I cmd-shift-N twice in a row I get 2 incognito windows.
Not sure how the dock menu item could behave differently.

http://codereview.chromium.org/196113/diff/8004/4005
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/196113/diff/8004/4005#newcode336
Line 336: browser->window()->Activate();
Good catch to do this from the dock, but we'd only want to do this when
triggered from the dock.  As an example, we don't want unit tests of Chrome to
grab focus.

http://codereview.chromium.org/196113/diff/8004/4006
File chrome/browser/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/196113/diff/8004/4006#newcode61
Line 61: [[NSApplication sharedApplication] activateIgnoringOtherApps:YES];
s/[NSApplication sharedApplication]/NSApp/g
Not sure this is a good idea; apps switching app focus is problematic (in fact,
Windows prevents it in many cases).

[vedran.sajatovic, 2009-09-16 21:56:30]

Hi,

> http://codereview.chromium.org/196113/diff/8004/4004#newcode472
> Line 472: Browser::OpenEmptyWindow(defaultProfile->GetOffTheRecordProfile());
> I don't believe this change makes any difference.
> If I cmd-shift-N twice in a row I get 2 incognito windows.
> Not sure how the dock menu item could behave differently.

It does behave differently. This is because the IDC_NEW_INCOGNITO_WINDOW command
is handled in two places: browser.cc (if there is an open browser window), and
app_controller_mac.mm (if there is no open browser window). browser.cc behaves
correctly and always opens a new incognito window, while app_controller_mac.mm
calls Browser::OpenURLOffTheRecord() (in browser.cc). If you take a closer look
at that method, you will see that it doesn't always open a new window (what I
would expect when clicking on "New Incognito Window"), but searches for an
existing incognito window, only in case no one is found creates one, and then
adds a tab to it.
Why hitting cmd-shift-N twice in a row works: Either there is an open window, so
the command is processed in browser.cc and a new incognito window is created, or
there is no open window, so the command is processed in app_controller_mac.mm,
no incognito window is found, and one is created. The next time you hit
cmd-shift-N, the first window has focus (if another application had focus, you
wouldn't be able to use the chrome menu or keyboard shortcuts), so the command
is processed in browser.cc correctly and your second incognito window spawns.
Things are different when using the dock menu: you can trigger the command while
there is an incognito window open, but another application has focus. In that
case, the command will be handled in app_controller_mac.mm, the existing
incognito window will be found, and the new tab added to it. No new incognito
window. That's why I changed this line.

> http://codereview.chromium.org/196113/diff/8004/4005#newcode336
> Line 336: browser->window()->Activate();
> Good catch to do this from the dock, but we'd only want to do this when
> triggered from the dock.  As an example, we don't want unit tests of Chrome to
> grab focus.

Good point. I'll figure something out.

> http://codereview.chromium.org/196113/diff/8004/4006#newcode61
> Line 61: [[NSApplication sharedApplication] activateIgnoringOtherApps:YES];
> s/[NSApplication sharedApplication]/NSApp/g
> Not sure this is a good idea; apps switching app focus is problematic (in
fact,
> Windows prevents it in many cases).

Unfortunately, makeKeyAndOrderFront won't do the trick if another application is
active (the other application will remain active). Any suggestions?

Btw. the incognito window doesn't have incognito colors until you activate it, I
guess it wasn't intended to ever be opened without being activated at the same
time. I won't file an issue for this, as it will resolve once we fix this up.

BR, Vedran

[John Grabowski, 2009-09-16 22:35:41]

On Wed, Sep 16, 2009 at 2:56 PM, <vedran.sajatovic@gmail.com> wrote:

> Hi,
>
>  http://codereview.chromium.org/196113/diff/8004/4004#newcode472
>> Line 472:
>> Browser::OpenEmptyWindow(defaultProfile->GetOffTheRecordProfile());
>> I don't believe this change makes any difference.
>> If I cmd-shift-N twice in a row I get 2 incognito windows.
>> Not sure how the dock menu item could behave differently.
>>
>
> It does behave differently. This is because the IDC_NEW_INCOGNITO_WINDOW
> command
> is handled in two places: browser.cc (if there is an open browser window),
> and
> app_controller_mac.mm (if there is no open browser window). browser.cc
> behaves
> correctly and always opens a new incognito window, while
> app_controller_mac.mm
> calls Browser::OpenURLOffTheRecord() (in browser.cc). If you take a closer
> look
> at that method, you will see that it doesn't always open a new window (what
> I
> would expect when clicking on "New Incognito Window"), but searches for an
> existing incognito window, only in case no one is found creates one, and
> then
> adds a tab to it.
> Why hitting cmd-shift-N twice in a row works: Either there is an open
> window, so
> the command is processed in browser.cc and a new incognito window is
> created, or
> there is no open window, so the command is processed in
> app_controller_mac.mm,
> no incognito window is found, and one is created. The next time you hit
> cmd-shift-N, the first window has focus (if another application had focus,
> you
> wouldn't be able to use the chrome menu or keyboard shortcuts), so the
> command
> is processed in browser.cc correctly and your second incognito window
> spawns.
> Things are different when using the dock menu: you can trigger the command
> while
> there is an incognito window open, but another application has focus. In
> that
> case, the command will be handled in app_controller_mac.mm, the existing
> incognito window will be found, and the new tab added to it. No new
> incognito
> window. That's why I changed this line.


Excellent sleuthing.  Perhaps add a condensed version of this block as a
comment in the code to make sure nobody switches it back.


>
>  http://codereview.chromium.org/196113/diff/8004/4005#newcode336
>> Line 336: browser->window()->Activate();
>> Good catch to do this from the dock, but we'd only want to do this when
>> triggered from the dock.  As an example, we don't want unit tests of
>> Chrome to
>> grab focus.
>>
>
> Good point. I'll figure something out.
>
>  http://codereview.chromium.org/196113/diff/8004/4006#newcode61
>> Line 61: [[NSApplication sharedApplication]
>> activateIgnoringOtherApps:YES];
>> s/[NSApplication sharedApplication]/NSApp/g
>> Not sure this is a good idea; apps switching app focus is problematic (in
>>
> fact,
>
>> Windows prevents it in many cases).
>>
>
> Unfortunately, makeKeyAndOrderFront won't do the trick if another
> application is
> active (the other application will remain active). Any suggestions?
>

Here's one idea.  The menu item as selected from the dock is really a
different action.  Perhaps add a new ID,
IDC_NEW_INCOGNITO_WINDOW_ACTIVATE_IGNORING_OTHER_APPS.  On windows it's the
same as IDC_NEW_INCOGNITO_WINDOW; on the Mac it also calls
activateIgnoringOtherApps:YES.

Interestingly, the Windows implementation of BrowserWindow::Activate()
eventually calls ::SetForegroundWindow() which does give the app focus in
some cases (not just the window).  So we have somewhat of a naming
problem; IDC_NEW_INCOGNITO_WINDOW_AND_ACTIVATE
is logically what we want... but IDC_NEW_INCOGNITO_WINDOW already does that
on Windows (but not on Mac).

I need to think about this more; I'm fine with a new ID but am less than
pleased with my naming suggestion and the x-plat ambiguity.  Maybe there is
nothing cleaner to do here.

jrg



>
> Btw. the incognito window doesn't have incognito colors until you activate
> it, I
> guess it wasn't intended to ever be opened without being activated at the
> same
> time. I won't file an issue for this, as it will resolve once we fix this
> up.
>
> BR, Vedran
>
>
>
> http://codereview.chromium.org/196113
>

[sgk, 2009-09-18 14:49:53]

+mark@chromium.org, who's been working on startup and activation recently.

Mark, any comments re: Vedran's investigation re: dock menu activation?