Prevent title-only panel from fully minimizing when attention is cleared if mouse is in the panel o…

Prevent title-only panel from fully minimizing when attention is cleared if mouse is in the panel or the panel is being dragged.

Added dragging tests related to minimized panels. Most of the changes are test related.

BUG=127584
TEST=new tests added


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

[Andrei, 2012-05-15 22:43:29]

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/docke...
File chrome/browser/ui/panels/docked_panel_strip.cc (right):

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/docke...
chrome/browser/ui/panels/docked_panel_strip.cc:480: && !IsMouseInPanel(panel))
So now if one clicks on such panel and then moves the mouse without releasing
it, the panel will stay in the title-only state, right? Or am I confused here?
Is that something we care about?
I am slightly worried that there is a part of code somewhere that makes an
assumption about panels not being in the title-only state unless they are
drawing attention.

[jennb, 2012-05-15 22:53:54]

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/docke...
File chrome/browser/ui/panels/docked_panel_strip.cc (right):

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/docke...
chrome/browser/ui/panels/docked_panel_strip.cc:480: && !IsMouseInPanel(panel))
On 2012/05/15 22:43:29, Andrei wrote:
> So now if one clicks on such panel and then moves the mouse without releasing
> it, the panel will stay in the title-only state, right? Or am I confused here?
> Is that something we care about?
> I am slightly worried that there is a part of code somewhere that makes an
> assumption about panels not being in the title-only state unless they are
> drawing attention.

If one clicks on a title-only panel and moves the mouse without releasing, that
panel will be the currently dragged panel. Those panels stay in title-only mode
while being dragged.

Panels can be in title-only mode any time the mouse hovers low enough to raise
up minimized panels. Not necessarily drawing attention. I'm confident we kept
that in mind throughout.

After the drag is finished, and the mouse moves away from the bottom of the
screen, any panels that are in title-only mode but not drawing attention will be
reduced back to 4-pixel minimized mode.

[jianli, 2012-05-15 22:59:47]

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/docke...
File chrome/browser/ui/panels/docked_panel_strip.cc (left):

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/docke...
chrome/browser/ui/panels/docked_panel_strip.cc:640: if (are_titlebars_up_ ==
bring_up)
Why do we need to remove this check?

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/docke...
File chrome/browser/ui/panels/docked_panel_strip.cc (right):

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/docke...
chrome/browser/ui/panels/docked_panel_strip.cc:480: && !IsMouseInPanel(panel))
nit: && should be placed at the end of last line.

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/docke...
chrome/browser/ui/panels/docked_panel_strip.cc:486: if
(dragging_panel_current_iterator_ != panels_.end() &&
Please comment this since it might be a bit confusing. When we drag up a docked
panel a little bit, the mouse will be indeed outside the panel.

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/docke...
chrome/browser/ui/panels/docked_panel_strip.cc:494: mouse_position.y() <=
bounds.right() &&
y()?

Can we call gfx::Rect::Contains?

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_drag_browsertest.cc (right):

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_browsertest.cc:432:
EXPECT_EQ(panel_new_bounds, panel->GetBounds());
Can you also verify that panel is still in title-only state?

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_browsertest.cc:437:
EXPECT_EQ(Panel::MINIMIZED, panel->expansion_state());
Can we have a test that we're dragging a title-only panel at the middle position
of the titlebar and the panel is still title-only when we release the mouse?

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_browsertest.cc:442:
IN_PROC_BROWSER_TEST_F(PanelDragBrowserTest, DragMinimizedDrawAttentionPanel) {
DragMinimizedDrawAttentionPanel => DragMinimizedDrawnAttentionPanel or
DragMinimizedPanelOnAttention

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_mouse_watcher.h (right):

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_mouse_watcher.h:32: // Returns current mouse
position.
Please comment that this would return the latest mouse position which might not
be used to trigger the mouse move notification yet.

[jennb, 2012-05-16 00:47:09]

Also fixed badness in test detected by linux try bot.

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/docke...
File chrome/browser/ui/panels/docked_panel_strip.cc (left):

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/docke...
chrome/browser/ui/panels/docked_panel_strip.cc:640: if (are_titlebars_up_ ==
bring_up)
On 2012/05/15 22:59:47, jianli wrote:
> Why do we need to remove this check?

We may need to bring down panels that are in title-only mode even if all panels
are not in titlebar-up-hover-mode. E.g. because we chose not to bring down a
panel after attention is cleared because the mouse is in the panel.

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/docke...
File chrome/browser/ui/panels/docked_panel_strip.cc (right):

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/docke...
chrome/browser/ui/panels/docked_panel_strip.cc:480: && !IsMouseInPanel(panel))
On 2012/05/15 22:59:47, jianli wrote:
> nit: && should be placed at the end of last line.

Done.

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/docke...
chrome/browser/ui/panels/docked_panel_strip.cc:486: if
(dragging_panel_current_iterator_ != panels_.end() &&
On 2012/05/15 22:59:47, jianli wrote:
> Please comment this since it might be a bit confusing. When we drag up a
docked
> panel a little bit, the mouse will be indeed outside the panel.

Done.

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/docke...
chrome/browser/ui/panels/docked_panel_strip.cc:494: mouse_position.y() <=
bounds.right() &&
On 2012/05/15 22:59:47, jianli wrote:
> y()?
> 

Corrected.

> Can we call gfx::Rect::Contains?

Contains does not include the right edge.

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_drag_browsertest.cc (right):

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_browsertest.cc:432:
EXPECT_EQ(panel_new_bounds, panel->GetBounds());
On 2012/05/15 22:59:47, jianli wrote:
> Can you also verify that panel is still in title-only state?

Done.

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_browsertest.cc:437:
EXPECT_EQ(Panel::MINIMIZED, panel->expansion_state());
On 2012/05/15 22:59:47, jianli wrote:
> Can we have a test that we're dragging a title-only panel at the middle
position
> of the titlebar and the panel is still title-only when we release the mouse?
 
Added check after FinishDragTitlebar() to verify title-only mode is still in
effect. That covers what you're asking for because mouse-release does not end
title-only mode; panel stays in title-only mode until mouse moves away from
panel.

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_drag_browsertest.cc:442:
IN_PROC_BROWSER_TEST_F(PanelDragBrowserTest, DragMinimizedDrawAttentionPanel) {
On 2012/05/15 22:59:47, jianli wrote:
> DragMinimizedDrawAttentionPanel => DragMinimizedDrawnAttentionPanel or
> DragMinimizedPanelOnAttention

DragMinimizedPanelWhileDrawingAttention

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_mouse_watcher.h (right):

http://codereview.chromium.org/10384192/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_mouse_watcher.h:32: // Returns current mouse
position.
On 2012/05/15 22:59:47, jianli wrote:
> Please comment that this would return the latest mouse position which might
not
> be used to trigger the mouse move notification yet.

Done.

[jianli, 2012-05-16 01:04:16]

lgtm

[jennb, 2012-05-16 17:43:47]

Jian - I reworked OnPanelAttentionStateChanged per our discussions yesterday
evening. Could you take another look even though you already lgtm'd?

Thanks!

[jianli, 2012-05-16 18:37:40]

lgtm

http://codereview.chromium.org/10384192/diff/1014/chrome/browser/ui/panels/do...
File chrome/browser/ui/panels/docked_panel_strip.cc (right):

http://codereview.chromium.org/10384192/diff/1014/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:481: // Maybe bring down the
titlebar now that panel is not drawing attention.
nit: This comment does not apply to the check below.

http://codereview.chromium.org/10384192/diff/1014/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:485: // Leave titlebar up when
dragging a panel.
nit: Indeed we mean that this panel is being dragged, not any panel is being
dragged.

[commit-bot: I haz the power, 2012-05-16 18:52:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennb@chromium.org/10384192/4002

[commit-bot: I haz the power, 2012-05-16 21:48:36]

Change committed as 137530