Choose the right window to switch to when a panel is deactivated on Windows.

Choose the right window to switch to when a panel is deactivated on Windows.

We choose the last active browser window that is not minimized.

BUG=none
TEST=browser tests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=104584

[Dmitry Titov, 2011-10-07 20:29:15]

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/native...
File chrome/browser/ui/panels/native_panel.h (right):

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/native...
chrome/browser/ui/panels/native_panel.h:92: virtual bool
VerifyTitlebarPaintedAsActive(bool as_active) = 0;
Does it verify that the panel is actually active or that the color of the
titlebar is correct? It's not clear from the description. If it is verifying
that the panel is really active, we could name the method VarifyIsActive() and
implement it by checking the color of titlebar. It could be a different check on
another platform.

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view.cc:85:
panel_->manager()->GetNextPanelToActivate(panel_.get());
This can be split in 2 methods - platform-independent part that activates next
window, perhaps implemented in Panel::Deactivate() itself, and
platform-dependent "deactivate app" which probably should be in win_util
somewhere... and called form Panel::Deactivate().

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:541: size_t max_index_delta =
std::max(panel_index, panels_.size() - panel_index);
This needs a comment. Why this specific math?

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:551: if (panel_index >= delta) {
would be more readable as 'panel_index - delta >= 0' because it is a guard
against negative index.

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.h (right):

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.h:75: BrowserWindow*
GetNextPanelToActivate(Panel* panel) const;
Naming suggestion: if it may return not only a Panel, then perhaps we should
name it GetNextWindowToActivate?

[jennb, 2011-10-07 20:34:16]

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browsertest.cc:1055: // When a panel is created,
it should be active at first.
This comment makes it sound like this is default behavior for Panels, when
really, CreatePanel is overriding the default behavior by creating an active
panel. Would prefer deleting this comment and adding an end-of-line comment
"Creates active panel" instead.

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browsertest.cc:1093: // Create 5 panels in the
following screen layout:
layout shows 6 panels

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:527: BrowserWindow*
PanelManager::GetNextPanelToActivate(Panel* panel) const {
Why do we need to find the next panel within the same extension? Could keep it
simple and just iterate through begin_last_active() and find the first that
isn't minimized.

[jianli, 2011-10-07 22:01:54]

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/native...
File chrome/browser/ui/panels/native_panel.h (right):

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/native...
chrome/browser/ui/panels/native_panel.h:92: virtual bool
VerifyTitlebarPaintedAsActive(bool as_active) = 0;
On 2011/10/07 20:29:15, Dmitry Titov wrote:
> Does it verify that the panel is actually active or that the color of the
> titlebar is correct? It's not clear from the description. If it is verifying
> that the panel is really active, we could name the method VarifyIsActive() and
> implement it by checking the color of titlebar. It could be a different check
on
> another platform.

We want to verify that the titlebar is painted correctly per its active state.
This is the reason I name it as ***PaintedAsActive. Checking whether the panel
is active can be done by calling Panel::IsActive.

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view.cc:85:
panel_->manager()->GetNextPanelToActivate(panel_.get());
On 2011/10/07 20:29:15, Dmitry Titov wrote:
> This can be split in 2 methods - platform-independent part that activates next
> window, perhaps implemented in Panel::Deactivate() itself, and
> platform-dependent "deactivate app" which probably should be in win_util
> somewhere... and called form Panel::Deactivate().

As discussed, we keep as it is since the only thing we can move to Panel is the
call to PanelManager::GetNextBrowserWindowToActivate. Since it is simply one
line of code, we rather keep it here for simplicity.

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browsertest.cc:1055: // When a panel is created,
it should be active at first.
On 2011/10/07 20:34:16, jennb wrote:
> This comment makes it sound like this is default behavior for Panels, when
> really, CreatePanel is overriding the default behavior by creating an active
> panel. Would prefer deleting this comment and adding an end-of-line comment
> "Creates active panel" instead. 

Done.

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browsertest.cc:1093: // Create 5 panels in the
following screen layout:
On 2011/10/07 20:34:16, jennb wrote:
> layout shows 6 panels

Done.

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:527: BrowserWindow*
PanelManager::GetNextPanelToActivate(Panel* panel) const {
On 2011/10/07 20:34:16, jennb wrote:
> Why do we need to find the next panel within the same extension? Could keep it
> simple and just iterate through begin_last_active() and find the first that
> isn't minimized.

Per discussion, we changed to activate the last active browser window that is
not minimized.

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:541: size_t max_index_delta =
std::max(panel_index, panels_.size() - panel_index);
On 2011/10/07 20:29:15, Dmitry Titov wrote:
> This needs a comment. Why this specific math? 

Not needed due to deactivation algorithm change.

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:551: if (panel_index >= delta) {
On 2011/10/07 20:29:15, Dmitry Titov wrote:
> would be more readable as 'panel_index - delta >= 0' because it is a guard
> against negative index.

Not needed due to deactivation algorithm change.

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.h (right):

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.h:75: BrowserWindow*
GetNextPanelToActivate(Panel* panel) const;
On 2011/10/07 20:29:15, Dmitry Titov wrote:
> Naming suggestion: if it may return not only a Panel, then perhaps we should
> name it GetNextWindowToActivate?

Renamed to GetNextBrowserWindowToActivate.

[jennb, 2011-10-07 22:08:30]

LGTM but wait for dimich

http://codereview.chromium.org/8183005/diff/10001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/8183005/diff/10001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:1088: std::string
panel_name_base("PanelTets");
typo

http://codereview.chromium.org/8183005/diff/10001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:1090:
CreatePanelWithBounds(panel_name_base + base::IntToString(i),
Does it break if they all have the same name?

http://codereview.chromium.org/8183005/diff/10001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:1131: // Minimizing active panel
P0 should cause activate lat active tabbed window.
typo

[jianli, 2011-10-07 22:11:37]

http://codereview.chromium.org/8183005/diff/10001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/8183005/diff/10001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:1090:
CreatePanelWithBounds(panel_name_base + base::IntToString(i),
On 2011/10/07 22:08:30, jennb wrote:
> Does it break if they all have the same name?

Nope. But it will make debugging easier.

[Dmitry Titov, 2011-10-07 22:26:41]

LGTM

Please consider the following, but I do not insist on it:

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/native...
File chrome/browser/ui/panels/native_panel.h (right):

http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/native...
chrome/browser/ui/panels/native_panel.h:92: virtual bool
VerifyTitlebarPaintedAsActive(bool as_active) = 0;
On 2011/10/07 22:01:54, jianli wrote:
> On 2011/10/07 20:29:15, Dmitry Titov wrote:
> > Does it verify that the panel is actually active or that the color of the
> > titlebar is correct? It's not clear from the description. If it is verifying
> > that the panel is really active, we could name the method VarifyIsActive()
and
> > implement it by checking the color of titlebar. It could be a different
check
> on
> > another platform.
> 
> We want to verify that the titlebar is painted correctly per its active state.
> This is the reason I name it as ***PaintedAsActive. Checking whether the panel
> is active can be done by calling Panel::IsActive.

Same logic here as VerifyDrawingAttention() - the method is actually implemented
differently on various platforms. On Views, it checks for colors, on OSX, it
checks for internal flag. It makes sense to name the method by 'what' it does
rather then 'how exactly does it do that'. In this particular case, you verify
that the native panel actually, really and for sure is drawing 'active'
decorations. The 'how' is 'by changing the color of the titlebar'. On OSX, we
might again check for internal flag because the colors are not pulled until we
actually draw real pixels.

[jianli, 2011-10-07 22:32:01]

On 2011/10/07 22:26:41, Dmitry Titov wrote:
> LGTM
> 
> Please consider the following, but I do not insist on it:
> 
>
http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/native...
> File chrome/browser/ui/panels/native_panel.h (right):
> 
>
http://codereview.chromium.org/8183005/diff/1/chrome/browser/ui/panels/native...
> chrome/browser/ui/panels/native_panel.h:92: virtual bool
> VerifyTitlebarPaintedAsActive(bool as_active) = 0;
> On 2011/10/07 22:01:54, jianli wrote:
> > On 2011/10/07 20:29:15, Dmitry Titov wrote:
> > > Does it verify that the panel is actually active or that the color of the
> > > titlebar is correct? It's not clear from the description. If it is
verifying
> > > that the panel is really active, we could name the method VarifyIsActive()
> and
> > > implement it by checking the color of titlebar. It could be a different
> check
> > on
> > > another platform.
> > 
> > We want to verify that the titlebar is painted correctly per its active
state.
> > This is the reason I name it as ***PaintedAsActive. Checking whether the
panel
> > is active can be done by calling Panel::IsActive.
> 
> Same logic here as VerifyDrawingAttention() - the method is actually
implemented
> differently on various platforms. On Views, it checks for colors, on OSX, it
> checks for internal flag. It makes sense to name the method by 'what' it does
> rather then 'how exactly does it do that'. In this particular case, you verify
> that the native panel actually, really and for sure is drawing 'active'
> decorations. The 'how' is 'by changing the color of the titlebar'. On OSX, we
> might again check for internal flag because the colors are not pulled until we
> actually draw real pixels.

Renamed to VerifyActiveState.