Minimized panels should not open on Windows when activated by the OS

Minimized panels should not open on Windows when activated by the OS

Minimized panels are not "minimized" from the OS point of view so they may receive focus when other windows are minimized or closed. This patch sets the flags on the minimized panels in such way that they are not activated by the OS but still appear in the Alt-Tab menu and on the taskbar.

BUG=102721
TEST=Manually verify that minimized panels do not open spontaneously when other windows are closed, but are accessible through the taskbar and Alt-Tab.


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

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

[jennb, 2012-02-29 05:16:16]

panel.cc is intended for platform-independent logic. The Windows logic should go
in panel_browser_view.cc. You could do something similar to
panel_browser_window_gtk.cc where the native panel registers for
chrome::NOTIFICATION_PANEL_CHANGED_EXPANSION_STATE. When panel_browser_view
receives the notification, it can do all this logic.

See panel_browser_window_gtk.cc::Observe() for example.

[jennb, 2012-02-29 17:46:01]

Might also have to watch for the LAYOUT_CHANGED notification, too, as panels
going into overflow are considered "minimized".

On 2012/02/29 05:16:16, jennb wrote:
> panel.cc is intended for platform-independent logic. The Windows logic should
go
> in panel_browser_view.cc. You could do something similar to
> panel_browser_window_gtk.cc where the native panel registers for
> chrome::NOTIFICATION_PANEL_CHANGED_EXPANSION_STATE. When panel_browser_view
> receives the notification, it can do all this logic.
> 
> See panel_browser_window_gtk.cc::Observe() for example.

[Andrei, 2012-03-01 22:32:41]

Reworked the patch to move the platform-dependent logic into the corresponding
platform-dependent files. Added a native panel interface and used it for all 3
platforms.

Jenn, I will use your PanelStrip::IsPanelMinimized() once that is checked in.

[jennb, 2012-03-01 23:21:28]

LGTM - did you want to wait for my cleanup patch first?

http://codereview.chromium.org/9463022/diff/7001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/9463022/diff/7001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_view.cc:307: // set the flags
"NoActivate" and "AppWindow" to make sure
s/set/Set

http://codereview.chromium.org/9463022/diff/7001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_view.cc:314:
::SetWindowLong(native_window, GWL_EXSTYLE,
Should be indented 2 spaces instead of 4. Continuation lines indent by 4.

[aburago_google.com, 2012-03-01 23:36:40]

On Thu, Mar 1, 2012 at 3:21 PM, <jennb@chromium.org> wrote:

> LGTM - did you want to wait for my cleanup patch first?
>

Yes I think so -- we don't need two different ways to tell whether a panel
is minimized :)


>
>
> http://codereview.chromium.**org/9463022/diff/7001/chrome/**
>
browser/ui/panels/panel_**browser_view.cc<http://codereview.chromium.org/9463022/diff/7001/chrome/browser/ui/panels/panel_browser_view.cc>
> File chrome/browser/ui/panels/**panel_browser_view.cc (right):
>
> http://codereview.chromium.**org/9463022/diff/7001/chrome/**
>
browser/ui/panels/panel_**browser_view.cc#newcode307<http://codereview.chromium.org/9463022/diff/7001/chrome/browser/ui/panels/panel_browser_view.cc#newcode307>
> chrome/browser/ui/panels/**panel_browser_view.cc:307: // set the flags
>
> "NoActivate" and "AppWindow" to make sure
> s/set/Set
>
> http://codereview.chromium.**org/9463022/diff/7001/chrome/**
>
browser/ui/panels/panel_**browser_view.cc#newcode314<http://codereview.chromium.org/9463022/diff/7001/chrome/browser/ui/panels/panel_browser_view.cc#newcode314>
> chrome/browser/ui/panels/**panel_browser_view.cc:314:
> ::SetWindowLong(native_window, GWL_EXSTYLE,
> Should be indented 2 spaces instead of 4. Continuation lines indent by
> 4.
>
>
http://codereview.chromium.**org/9463022/<http://codereview.chromium.org/9463...
>

[Andrei, 2012-03-02 21:23:56]

Took care of the nits and made it use IsPanelMinimized() as in the cleanup
patch.

[jennb, 2012-03-02 21:30:06]

Still LGTM

[commit-bot: I haz the power, 2012-03-02 22:00:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aburago@chromium.org/9463022/9001

[commit-bot: I haz the power, 2012-03-03 00:38:45]

Change committed as 124790

[Andrei, 2012-03-06 17:56:17]

Added proper #if's for Aura.
Merged with the cleanup patch.
Added guards for NULL panel strip.

[jennb, 2012-03-06 21:56:44]

http://codereview.chromium.org/9463022/diff/18001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/9463022/diff/18001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.cc:164:
native_panel_->PreventActivationByOS(panel_strip_->IsPanelMinimized(this));
This makes more sense in AddPanel where the panel strip is already setting
various properties on the panel? That will require Panel.h to add a
PreventActivationByOS() as strips never access native_panel directly.

http://codereview.chromium.org/9463022/diff/18001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.cc:179: DCHECK(initialized_ && panel_strip_ !=
NULL);
Move this to the beginning of the method? Both these should be true when this
method is called.

http://codereview.chromium.org/9463022/diff/18001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel.h (right):

http://codereview.chromium.org/9463022/diff/18001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.h:225: void set_panel_strip(PanelStrip*
new_strip);
FYI - style guide only allows the lowercase naming for basic getters/setters.
Otherwise, this needs to be SetPanelStrip() instead of set_panel_strip().

[Andrei, 2012-03-06 22:18:40]

On Tue, Mar 6, 2012 at 1:56 PM, <jennb@chromium.org> wrote:

>
> http://codereview.chromium.**org/9463022/diff/18001/chrome/**
>
browser/ui/panels/panel.cc<http://codereview.chromium.org/9463022/diff/18001/chrome/browser/ui/panels/panel.cc>
> File chrome/browser/ui/panels/**panel.cc (right):
>
> http://codereview.chromium.**org/9463022/diff/18001/chrome/**
>
browser/ui/panels/panel.cc#**newcode164<http://codereview.chromium.org/9463022/diff/18001/chrome/browser/ui/panels/panel.cc#newcode164>
> chrome/browser/ui/panels/**panel.cc:164:
> native_panel_->**PreventActivationByOS(panel_**
> strip_->IsPanelMinimized(this)**);
> This makes more sense in AddPanel where the panel strip is already
> setting various properties on the panel? That will require Panel.h to
> add a PreventActivationByOS() as strips never access native_panel
> directly.
>

Yes, I was trying to avoid adding a Panel::PreventActivationByOS() method.
The way it is done now, this is restricted to interaction between panel and
native panel, and no one else needs to know.
A slight drawback is that we need to add code both to Initialize() and
set_panel_strip(), since we do not know which will be canned first. Still,
limiting the scope felt like a bigger advantage to me.  Do you feel
differently?

>
> http://codereview.chromium.**org/9463022/diff/18001/chrome/**
>
browser/ui/panels/panel.cc#**newcode179<http://codereview.chromium.org/9463022/diff/18001/chrome/browser/ui/panels/panel.cc#newcode179>
> chrome/browser/ui/panels/**panel.cc:179: DCHECK(initialized_ &&
> panel_strip_ != NULL);
> Move this to the beginning of the method? Both these should be true when
> this method is called.
>

Right, but I moved it immediately before these values are used. This is the
place where they become relevant, and everyone can see why we require them.
I can move it to the top of the method if you think it's better style. :)


>
> http://codereview.chromium.**org/9463022/diff/18001/chrome/**
>
browser/ui/panels/panel.h<http://codereview.chromium.org/9463022/diff/18001/chrome/browser/ui/panels/panel.h>
> File chrome/browser/ui/panels/**panel.h (right):
>
> http://codereview.chromium.**org/9463022/diff/18001/chrome/**
>
browser/ui/panels/panel.h#**newcode225<http://codereview.chromium.org/9463022/diff/18001/chrome/browser/ui/panels/panel.h#newcode225>
> chrome/browser/ui/panels/**panel.h:225: void set_panel_strip(PanelStrip*
> new_strip);
> FYI - style guide only allows the lowercase naming for basic
> getters/setters. Otherwise, this needs to be SetPanelStrip() instead of
> set_panel_strip().
>

Jenn, I did not name this method, actually. You did :) :) I can change the
name however if you'd like :)


>
>
http://codereview.chromium.**org/9463022/<http://codereview.chromium.org/9463...
>

[Andrei, 2012-03-07 01:23:19]

renamed set_panel_strip() to SetPanelStrip() to comply with the style guide.

[jennb, 2012-03-07 18:30:49]

LGTM

http://codereview.chromium.org/9463022/diff/27001/chrome/browser/ui/panels/ov...
File chrome/browser/ui/panels/overflow_panel_strip.cc (right):

http://codereview.chromium.org/9463022/diff/27001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_strip.cc:9: #include
"chrome/browser/ui/panels/native_panel.h"
delete

[commit-bot: I haz the power, 2012-03-07 21:22:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aburago@chromium.org/9463022/31001

[commit-bot: I haz the power, 2012-03-08 02:57:03]

Try job failure for 9463022-31001 (retry) on win_rel for step "ui_tests".
It's a second try, previously, step "ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-08 16:58:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aburago@chromium.org/9463022/31001

[commit-bot: I haz the power, 2012-03-08 21:18:13]

Try job failure for 9463022-31001 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-08 23:36:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aburago@chromium.org/9463022/31001

[commit-bot: I haz the power, 2012-03-09 01:12:02]

Change committed as 125752