Make double-clicking on the tab strip minimize (i.e., "miniaturize") the...

Make double-clicking on the tab strip minimize (i.e., "miniaturize") the
window on Mac. This entails three things:
  - getting rid of the hitTest()-hack in TabStripView (which fixed bug
    12505);
  - capturing mouse-up events (and looking for double-clicks); and
  - making TabStripView accept first mouse (to fix bug 12505 again).
A kludge is needed to prevent double-clicks on, e.g., the zoom button
from causing a minimize. (The hard-coded timestamp-based kludge was
chosen as the least of all evils, though perhaps there's a better way to
do this. A button-hit-test kludge was considered, but this is hard,
since hitting the zoom button the first time may cause the window to
move!)

Patch set 2: Properly "flashes" minimize button (by using
-performMiniaturize instead of -miniaturize). Also checks user
preferences as to whether double-clicks on title bars should minimize.
Updated variable/member names to comply with style guide for Obj-C/Cocoa
code.

Patch set 3: Use the undocumented method +[NSWindow
_shouldMiniaturizeOnDoubleClick] (but run-time check and DCHECK() its
existence), instead of an undocumented user pref key.

BUG=12286
TEST=none

[Nico, 2009-07-19 17:33:32]

I don't like this patch: In my eyes, being able to minimize by clicking the
non-tabstrip window title to minimize is sufficient and already works today, and
this patch is hacky. But that's not my decision to make.

If people decide to go forward with this (if you haven't done so yet, discuss
this patch on irc in #chromium, the mac folks are around mostly 8-14 pst),
please make sure that this new functionality is only active if "Minimize when
double-clicking a window title bar" is checked in System
Preferences->Appearance.

[viettrungluu-gmail, 2009-07-19 19:40:20]

On 2009/07/19 17:33:32, thakis wrote:
> I don't like this patch: In my eyes, being able to minimize by clicking the
> non-tabstrip window title to minimize is sufficient and already works today,
and
> this patch is hacky. But that's not my decision to make.

I agree that it's hacky. In defense of it, on the UI side, there's no visual
boundary between the window titlebar area proper and the tabstrip (in fact, the
way the UI is designed, the tabstrip lives partly in where the usual titlebar
is). On the consistency-with-other-apps side, Safari is kind of similarly
designed, and double-clicking, e.g., below the location box works there.

BUG in patch: I just noticed that the other apps depress the window miniaturize
button when you double-click the titlebar.

> please make sure that this new functionality is only active if "Minimize when
> double-clicking a window title bar" is checked in System
> Preferences->Appearance.

Also something I missed, thanks. Probably I shouldn't miniaturize directly (to
fix this and the above).

[viettrungluu-gmail, 2009-07-20 02:19:37]

Patch set 2 fixes all aforementioned issues, except for the fact that it's a
hack (sorry, can't seem to do anything about that, given Apple's public API).

The "proper" way to go about this might be to make the tab strip be the
"toolbar" of the browser window (with the NSUnifiedTitleAndToolbarWindowMask
attribute). This is surely how Safari and other apps deal with this (yes, you
can double-click on their blank toolbar areas to minimize).

[viettrungluu-gmail, 2009-07-22 00:36:18]

(Following up on discussion with thakis and pinkerton on #chromium.)

Here are the gory details about how double-clicks are handled (with and without
a toolbar -- the cases are different), and how the window views are organized
when a toolbar is present.

First, the views: the window's root view is some sort of NS...Frame (probably
some subclass of NSTitledFrame; this is all Apple-private). This view in turn
has as subviews a bunch of widgets -- these are the three standard buttons;
there is no title bar view! It has the content view, of course. If applicable,
it has an NSToolbarView (Apple-private), which in turn contains a bunch of other
views. (Side comment: Apple obviously doesn't want anyone mucking around with
the toolbar, so hacking it to contain the tabs would be nontrivial.) The
NSToolbarView's frame rect is disjoint from the title bar, so it handles its own
clicks.

The stack trace after a double-click on the toolbar:
#0  0x9290beeb in -[NSWindow miniaturize:] ()
#1  0x927134cb in -[NSApplication sendAction:to:from:] ()
#2  0x92713408 in -[NSControl sendAction:to:] ()
#3  0x9271328e in -[NSCell _sendActionFrom:] ()
#4  0x927ed53b in -[NSButtonCell performClick:] ()
#5  0x928d243e in -[NSToolbarView _mouseUpWithEvent:forView:] ()
#6  0x928dc3f4 in -[NSToolbarItemViewer mouseUp:] ()
#7  0x927101d1 in -[NSWindow sendEvent:] ()
#8  0x926dccd9 in -[NSApplication sendEvent:] ()
#9  0x9263a62f in -[NSApplication run] ()
...

The NSButtonCell referred to is the minimize button; this is no different from
calling -[NSWindow performMiniaturize:].

The stack trace when double-clicked on the title bar:
#0  0x9290beeb in -[NSWindow miniaturize:] ()
#1  0x927134cb in -[NSApplication sendAction:to:from:] ()
#2  0x92713408 in -[NSControl sendAction:to:] ()
#3  0x9271328e in -[NSCell _sendActionFrom:] ()
#4  0x927ed53b in -[NSButtonCell performClick:] ()
#5  0x928b0464 in -[NSTitledFrame mouseUp:] ()
#6  0x927101d1 in -[NSWindow sendEvent:] ()
#7  0x926dccd9 in -[NSApplication sendEvent:] ()
#8  0x9263a62f in -[NSApplication run] ()
...

The toolbar's code doesn't require any sort of timer (or other) hack since it
doesn't overlap the title bar, so all double-clicks on its empty area are good.
By stepping through the code, I've learned that one can check whether one should
minimize on double-click by calling (the Apple-private method) +[NSWindow
_shouldMiniaturizeOnDoubleClick]. We can check whether or not NSWindow responds
to it, and use it if it does. (The advantage: It's much more efficient, and
avoids reading the equally-undocumented user preference
AppleMiniaturizeOnDoubleClick.)

[pink (ping after 24hrs), 2009-07-22 12:11:57]

Ok, I think you've convinced me that the tab strip needs to be the one handling
minimization manually.

In the future, this type of information is better to put in the relevant bug
rather than a particular patch where it's easy to get lost. Thanks again for
doing all this research!!!!!

[Nico, 2009-07-22 18:25:54]

LGTM from me, too. Looks like this is the best we can do.

Thanks for the research! Perhaps you can copy your last comment on this CL to
the bug this CL is for.

[pink (ping after 24hrs), 2009-07-22 18:55:24]

Mostly LGTM but there's a bunch of style/grammar stuff to fix up first.

http://codereview.chromium.org/159062/diff/11/12
File chrome/browser/cocoa/tab_strip_view.h (right):

http://codereview.chromium.org/159062/diff/11/12#newcode13
Line 13: @private
indent @private one space.

http://codereview.chromium.org/159062/diff/11/13
File chrome/browser/cocoa/tab_strip_view.mm (right):

http://codereview.chromium.org/159062/diff/11/13#newcode5
Line 5: #include "chrome/browser/cocoa/tab_strip_view.h"
this can be #import because it's an obj-c header.

http://codereview.chromium.org/159062/diff/11/13#newcode6
Line 6: #include "base/logging.h"
keep these alphabetized. base comes before chrome.

http://codereview.chromium.org/159062/diff/11/13#newcode13
Line 13: // So that a valid timestamp - lastMouseUp_ is at least 1000 unless it
has
This is a strange sentence fragment. Use complete sentences.

http://codereview.chromium.org/159062/diff/11/13#newcode29
Line 29: // We accept first mouse so clicks onto close/zoom/miniaturize buttons
and title bar double-clicks are properly detected even when the window is in
restrict lines to 80 columns

http://codereview.chromium.org/159062/diff/11/13#newcode30
Line 30: // the background. This supercedes the hitTest: hack. We also need to
do this
don't mention code that isn't here. Once it's gone, nobody will know to what
it's referring.

[viettrungluu-gmail, 2009-07-22 19:16:40]

On 2009/07/22 18:55:24, pink wrote:
> File chrome/browser/cocoa/tab_strip_view.mm (right):
> Line 6: #include "base/logging.h"
> keep these alphabetized. base comes before chrome.

We'll #include/#import its own header first, but I added a newline to make this
clearer.

Everything else fixed. Thanks for the review.

[pink (ping after 24hrs), 2009-07-22 19:42:49]

pushed as r21308