Fix the bug that minimized panel window might steal focus on Mac

Fix the bug that minimized panel window might steal focus on Mac

This is caused by the problem that the minimized panel could still become a key window. We found out a way to deactivate a NSWindow correctly.

BUG=365758
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272420

[Dmitry Titov, 2014-05-16 16:37:38]

As I understand, you are removing resizable mask because a window w/o resizable
or title masks can not take focus :
https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Applica...

However, I don't think I understand the early return in windowDidBecomeKey ,
could you clarify?

https://codereview.chromium.org/275693002/diff/1/chrome/browser/ui/cocoa/pane...
File chrome/browser/ui/cocoa/panels/panel_window_controller_cocoa.mm (right):

https://codereview.chromium.org/275693002/diff/1/chrome/browser/ui/cocoa/pane...
chrome/browser/ui/cocoa/panels/panel_window_controller_cocoa.mm:591: // Prevent
the minimized panel from invoking any activation logic. This is
I'm not sure I understand the change being made. If the window is receiving
"didBecomeKey", doesn't it mean the OS already has transferred focus to it?
Would it work if we implement -(BOOL)canBecomeKeyWindow and return NO if it is
minimized?

If you are trying to avoid setting a resizable window mask when the window is
minimized, then perhaps this return should be moved to later in this function.
It feels strange not to activate RWHV on windowDidBecomeKey...

[jianli, 2014-05-20 00:56:00]

https://codereview.chromium.org/275693002/diff/1/chrome/browser/ui/cocoa/pane...
File chrome/browser/ui/cocoa/panels/panel_window_controller_cocoa.mm (right):

https://codereview.chromium.org/275693002/diff/1/chrome/browser/ui/cocoa/pane...
chrome/browser/ui/cocoa/panels/panel_window_controller_cocoa.mm:591: // Prevent
the minimized panel from invoking any activation logic. This is
On 2014/05/16 16:37:38, Dmitry Titov wrote:
> I'm not sure I understand the change being made. If the window is receiving
> "didBecomeKey", doesn't it mean the OS already has transferred focus to it?
> Would it work if we implement -(BOOL)canBecomeKeyWindow and return NO if it is
> minimized?
> 
> If you are trying to avoid setting a resizable window mask when the window is
> minimized, then perhaps this return should be moved to later in this function.
> It feels strange not to activate RWHV on windowDidBecomeKey...

We do instruct canBecomeKeyWindow  to return NO but it is never called by the
system when the window is already a key window.

I found another way to deactivate a NSWindow programatically.

[Dmitry Titov, 2014-05-20 01:35:32]

reluctant lgtm.

Please give it a lot of manual testing, since other behavior may be broken...
- with and w/o Chrome windows
- more then on Chrome window
- more then one panel
- panel with focus closed
- last Chrome window with focus closed
- with both docked and stacked panels

I have vague recollection we already did orderOut/orderFront and for some reason
we ended up not using it. Lets give it a lot of test...

[jianli, 2014-05-20 02:12:23]

On 2014/05/20 01:35:32, Dmitry Titov wrote:
> reluctant lgtm.
> 
> Please give it a lot of manual testing, since other behavior may be broken...
> - with and w/o Chrome windows
> - more then on Chrome window
> - more then one panel
> - panel with focus closed
> - last Chrome window with focus closed
> - with both docked and stacked panels
> 
> I have vague recollection we already did orderOut/orderFront and for some
reason
> we ended up not using it. Lets give it a lot of test...

I will do more manual testing on this.

I just checked revision history. It seemed that
http://src.chromium.org/viewvc/chrome?view=revision&revision=106832 was the
first version to introduce the logic to deactivate the app. Probably
orderOut/orderFront was used for other scenarios, like fullscreen mode support.

[jianli, 2014-05-20 20:15:38]

Did a lot of manual testings to make sure everything works all right. Found only
one problem with collapsing a stacked panel and got the fix for it. Dimich,
could you please review again? Thanks.

[Dmitry Titov, 2014-05-20 20:58:05]

lgtm, thanks for testing it!

[jianli, 2014-05-20 20:59:02]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2014-05-20 20:59:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jianli@chromium.org/275693002/40001

[commit-bot: I haz the power, 2014-05-21 00:31:09]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-21 06:32:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-21 06:32:42]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[jianli, 2014-05-22 21:28:22]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2014-05-22 21:31:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jianli@chromium.org/275693002/60001

[commit-bot: I haz the power, 2014-05-23 00:58:28]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-23 07:06:13]

Change committed as 272420