Change Panel titlebars to activate the panel on click (rather than minimize).

Change Panel titlebars to activate the panel on click (rather than minimize).

Ctrl/Shift+titlebar-click on a minimized panel continues to restore-all, but
does not minimize-all on an expanded panel anymore.

An explicit minimize/restore button will be used for minimize/restore (separate patches), thus clicking on the titlebar is being changed to focus/restore the panel instead.

BUG=101217, 118431, 121020, 123176
TEST=Existing tests updated, also verified manually on all platforms.

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

[jennb, 2012-04-17 23:51:54]

I won't commit this until we have an alternate way to minimize a panel, but
please review in the meanwhile. Ignore earlier patches which were
work-in-progress patches as I moved between machines.

Dimich - GTK and Cocoa changes
Jianli - Win changes

[Dmitry Titov, 2012-04-18 01:24:37]

Ouch! I've added comments but the patch set got deleted, with them!
Re-adding...

[Dmitry Titov, 2012-04-18 01:49:45]

http://codereview.chromium.org/10106008/diff/2003/chrome/browser/ui/panels/de...
File chrome/browser/ui/panels/detached_panel_strip.cc (right):

http://codereview.chromium.org/10106008/diff/2003/chrome/browser/ui/panels/de...
chrome/browser/ui/panels/detached_panel_strip.cc:68: panel->Activate();
See the comment in the same method of the DockStrip...

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

http://codereview.chromium.org/10106008/diff/2003/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:431: panel->Deactivate();
We should reliably prevent activation while dragging the title-only panel. I
don't think we can guarantee (at least not for all OSes) that in all conditions
the Activate->Deactivate sequence, if it happens, will leave the order of
windows and activation where it was, especially when other apps are in the
picture. 
We should DCHECK here and make sure we don't activate in this case.

http://codereview.chromium.org/10106008/diff/2003/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:512: panel->Activate();  // Will
expand a minimized panel.
We should activate on mousedown. This is the 'normal' behavior on all OSes and
we do this to brin g'normal' behavior to panels.
Also, on some OSes (all?) the mosuedown is wired to some system action that
activates the window - normally, apps do not need to react to clicks in their
windows by calling some Activate() method... We should just let those things
work (we suppressed them before)
If we don't need to actually Activate() here, we still might want to Expand() a
minimized panel - but we should do that explicitly then, not using the
Activate() method.

[jianli, 2012-04-18 22:18:47]

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

http://codereview.chromium.org/10106008/diff/2003/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:512: panel->Activate();  // Will
expand a minimized panel.
Just as dimich pointed out, Activate call is not needed on Windows since it is
automatically handled by the system.

http://codereview.chromium.org/10106008/diff/2003/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/10106008/diff/2003/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:505: if
(base::win::IsCtrlPressed()) {
nit: {} is not needed.

http://codereview.chromium.org/10106008/diff/2003/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:506: click_modifier =
panel::APPLY_TO_ALL;
Since we're not longer supporting click-to-minimize, do we want also change the
pattern for minimize-all? Like, holding ctrl while clicking on minimize button?

[jennb, 2012-04-19 16:30:59]

On Wed, Apr 18, 2012 at 3:18 PM, <jianli@chromium.org> wrote:

>
> http://codereview.chromium.**org/10106008/diff/2003/chrome/**
>
browser/ui/panels/panel_**browser_view.cc#newcode506<http://codereview.chromium.org/10106008/diff/2003/chrome/browser/ui/panels/panel_browser_view.cc#newcode506>
> chrome/browser/ui/panels/**panel_browser_view.cc:506: click_modifier =
> panel::APPLY_TO_ALL;
> Since we're not longer supporting click-to-minimize, do we want also
> change the pattern for minimize-all? Like, holding ctrl while clicking
> on minimize button?
>
>
Good question. Need to ask Ken. Any changes for it would be a separate
patch, of course.

[jennb, 2012-04-24 18:47:14]

Finally ready for another round of review.

http://codereview.chromium.org/10106008/diff/2003/chrome/browser/ui/panels/de...
File chrome/browser/ui/panels/detached_panel_strip.cc (right):

http://codereview.chromium.org/10106008/diff/2003/chrome/browser/ui/panels/de...
chrome/browser/ui/panels/detached_panel_strip.cc:68: panel->Activate();
On 2012/04/18 01:49:46, Dmitry Titov wrote:
> See the comment in the same method of the DockStrip...

Reverted.

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

http://codereview.chromium.org/10106008/diff/2003/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:431: panel->Deactivate();
On 2012/04/18 01:49:46, Dmitry Titov wrote:
> We should reliably prevent activation while dragging the title-only panel. I
> don't think we can guarantee (at least not for all OSes) that in all
conditions
> the Activate->Deactivate sequence, if it happens, will leave the order of
> windows and activation where it was, especially when other apps are in the
> picture. 
> We should DCHECK here and make sure we don't activate in this case.

Moved work-around into PanelBrowserView as it is Windows-specific.

On Windows, mouse down immediately activates a window so there's no opportunity
to prevent activation while dragging a title-only panel. This is only a problem
when drawing attention because after dragging a non-drawing attention minimized
panel, there's an expansion state change from TITLE_ONLY back to MINIMIZED,
which calls Deactivate(). There is no expansion state change after dragging a
minimized panel that is drawing attention, so we're left with an activated
minimized panel. It's an edge case. 

Jian tells me that we can jump through some hoops on Windows to prevent being
activated from a mouse-press while minimized, but that should be attempted in a
separate patch if we attempt it at all.

http://codereview.chromium.org/10106008/diff/2003/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:512: panel->Activate();  // Will
expand a minimized panel.
On 2012/04/18 01:49:46, Dmitry Titov wrote:
> We should activate on mousedown. This is the 'normal' behavior on all OSes and
> we do this to brin g'normal' behavior to panels.
> Also, on some OSes (all?) the mosuedown is wired to some system action that
> activates the window - normally, apps do not need to react to clicks in their
> windows by calling some Activate() method... We should just let those things
> work (we suppressed them before)
> If we don't need to actually Activate() here, we still might want to Expand()
a
> minimized panel - but we should do that explicitly then, not using the
> Activate() method.

Removed. See adjustments to Panel::OnTitlebarClicked as a result.

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_window_controller_cocoa.mm (right):

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_window_controller_cocoa.mm:108: return ([app
isHandlingSendEvent]  && [[app currentEvent] window] == self) ||
FYI: this change is the fix needed to prevent tabbed browser window from jumping
forward when clicking on a panel titlebar. It's needed because the system checks
-canBecomeKeyWindow when titlebar is clicked *before* we get
-NSWindow:sendEvent. Conveniently, this also eliminates the need for overriding
-sendEvent.

[jianli, 2012-04-25 18:19:31]

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel.cc:690: // Clear draw attention state when an
expanded panel becomes active.
remove "draw" in the comment?

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_view.cc:485: panel_->Deactivate();
It might be better if we can figure out a way to avoid activating a minimize
panel and then deactivating it on mouse release. Can you put a TODO here?

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browser_view.h (right):

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_view.h:11: #include "base/time.h"
Can be removed.

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_view.h:170: base::TimeTicks
mouse_pressed_time_;
This can be removed now.

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browsertest.cc:555: // to minimize all panels.
Nothing changes.
Please update the comment for "minimize all panels". Probably something like:
Clicking titlebar on expanded panel will only activate the panel. Clicking
titlebar with a modifier is same as clicking without a modifier.

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browsertest.cc:563: // Minimize all panels. Then
click on a panel with apply-all modifier.
Better comment like: Clicking titlebar on minimized panel will expand the panel.
Clicking titlebar with a apply-all modifier will expand all panels.

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_window_controller_cocoa.mm (right):

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_window_controller_cocoa.mm:14: #include
"base/time.h"
Can be removed.

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_window_controller_cocoa.mm:38: #include
"chrome/common/chrome_notification_types.h"
ditto.

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_window_controller_cocoa.mm:39: #include
"content/public/browser/notification_service.h"
ditto.

[jennb, 2012-04-25 20:34:05]

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel.cc:690: // Clear draw attention state when an
expanded panel becomes active.
On 2012/04/25 18:19:31, jianli wrote:
> remove "draw" in the comment?

Done.

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_view.cc:485: panel_->Deactivate();
On 2012/04/25 18:19:31, jianli wrote:
> It might be better if we can figure out a way to avoid activating a minimize
> panel and then deactivating it on mouse release. Can you put a TODO here?

crbug created instead.

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browser_view.h (right):

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_view.h:11: #include "base/time.h"
On 2012/04/25 18:19:31, jianli wrote:
> Can be removed.

Done.

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_view.h:170: base::TimeTicks
mouse_pressed_time_;
On 2012/04/25 18:19:31, jianli wrote:
> This can be removed now.

Done.

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browsertest.cc:555: // to minimize all panels.
Nothing changes.
On 2012/04/25 18:19:31, jianli wrote:
> Please update the comment for "minimize all panels". Probably something like:
> Clicking titlebar on expanded panel will only activate the panel. Clicking
> titlebar with a modifier is same as clicking without a modifier.

Edited.

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browsertest.cc:563: // Minimize all panels. Then
click on a panel with apply-all modifier.
On 2012/04/25 18:19:31, jianli wrote:
> Better comment like: Clicking titlebar on minimized panel will expand the
panel.
> Clicking titlebar with a apply-all modifier will expand all panels.

Edited.

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_window_controller_cocoa.mm (right):

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_window_controller_cocoa.mm:14: #include
"base/time.h"
On 2012/04/25 18:19:31, jianli wrote:
> Can be removed.

Done.

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_window_controller_cocoa.mm:38: #include
"chrome/common/chrome_notification_types.h"
On 2012/04/25 18:19:31, jianli wrote:
> ditto.

Done.

http://codereview.chromium.org/10106008/diff/42001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_window_controller_cocoa.mm:39: #include
"content/public/browser/notification_service.h"
On 2012/04/25 18:19:31, jianli wrote:
> ditto.

Done.

[Dmitry Titov, 2012-04-25 21:37:35]

lgtm.

[jianli, 2012-04-25 22:30:46]

lgtm