Follow El Capitan Dock settings for double-click in window title bar.

Follow El Capitan Dock settings for double-click in window title bar.

In El Capitan there are Dock settings governing what happens when you
double-click in a window's title. This change allows browser windows
to behave like other windows on the system that respect this setting.

BUG=576520
Committed: https://crrev.com/459a5b538a486568bda23d098aec5a430c6ba1fe
Cr-Commit-Position: refs/heads/master@{#370726}

[shrike, 2016-01-15 22:36:56]

Description was changed from

==========
Follow El Capitan Dock settings for double-click in window title bar.

In El Capitan there are Dock settings governing what happens when you
double-click in a window's title. This change allows browser windows
to behave like other windows on the system that respect this setting.

BUG=576520
==========

to

==========
Follow El Capitan Dock settings for double-click in window title bar.

In El Capitan there are Dock settings governing what happens when you
double-click in a window's title. This change allows browser windows
to behave like other windows on the system that respect this setting.

BUG=576520
==========

[shrike, 2016-01-15 22:36:56]

shrike@chromium.org changed reviewers:
+ tapted@chromium.org

[shrike, 2016-01-15 22:37:06]

PTAL

[tapted, 2016-01-17 23:53:18]

lgtm % nits

https://codereview.chromium.org/1588073005/diff/1/ui/base/cocoa/appkit_utils.mm
File ui/base/cocoa/appkit_utils.mm (right):

https://codereview.chromium.org/1588073005/diff/1/ui/base/cocoa/appkit_utils....
ui/base/cocoa/appkit_utils.mm:35: if (!base::mac::IsOSYosemiteOrLater()) {
nit: IsOSMavericksOrEarlier() ?

https://codereview.chromium.org/1588073005/diff/1/ui/base/cocoa/appkit_utils....
ui/base/cocoa/appkit_utils.mm:41: // In El Capitan there's a Dock setting that
governs the action.
nit: Maybe something like "El Capitan introduced a checkbox to disable the
double-click action, as well as switch between Minimize and Zoom."  (otherwise
it's confusing why we don't select for Zoom here as well -- e.g. maybe we
shouldn't use ShouldWindows*Miniaturize*OnDoubleClick() on El Capitan at all,
but what you have seems neat enough. It looks like they haven't killed the
_shouldMiniaturizeOnDoubleClick method yet.)

[shrike, 2016-01-19 19:34:23]

PTAL

https://codereview.chromium.org/1588073005/diff/1/ui/base/cocoa/appkit_utils.mm
File ui/base/cocoa/appkit_utils.mm (right):

https://codereview.chromium.org/1588073005/diff/1/ui/base/cocoa/appkit_utils....
ui/base/cocoa/appkit_utils.mm:35: if (!base::mac::IsOSYosemiteOrLater()) {
On 2016/01/17 23:53:18, tapted wrote:
> nit: IsOSMavericksOrEarlier() ?

I guess I like !IsOSYosemiteOrLater() because the code is saying for everything
before Yosemite, return false. Saying 

  if (base::mac::IsOSMavericksOrEarlier()) {
    return false;
  } else if (base::mac::IsOSYosemite()) {
    return true;
  }

means that everything after Yosemite, and in between Mavericks and Yosemite,
falls through. I myself have a little trouble remembering the OSes in between
MountainLion and Yosemite, so to me my version is more clear.

https://codereview.chromium.org/1588073005/diff/1/ui/base/cocoa/appkit_utils....
ui/base/cocoa/appkit_utils.mm:41: // In El Capitan there's a Dock setting that
governs the action.
On 2016/01/17 23:53:18, tapted wrote:
> nit: Maybe something like "El Capitan introduced a checkbox to disable the
> double-click action, as well as switch between Minimize and Zoom."  (otherwise
> it's confusing why we don't select for Zoom here as well -- e.g. maybe we
> shouldn't use ShouldWindows*Miniaturize*OnDoubleClick() on El Capitan at all,
> but what you have seems neat enough. It looks like they haven't killed the
> _shouldMiniaturizeOnDoubleClick method yet.)

Good point. I added your comment language, and also changed
ShouldWindowsMiniaturizeOnDoubleClick() to read the Dock preference under El
Capitan. Originally I decided against doing that because the undocumented method
seems to still work, but explicitly checking the preference on El Capitan and
above is more clear, and future proofs us against Apple removing the
undocumented API.

[tapted, 2016-01-19 22:16:35]

lgtm 

A small part of me is inclined to suggest an enum to avoid a double-lookup in
prefs, but it might end up clumsy with all the OS-version checking. I think what
you have is perfectly readable, and the rest is just bikeshedding :)

(also it's so weird they chose Minimize/Maximize as their pref string instead of
Miniaturize/Zoom - oh well).

https://codereview.chromium.org/1588073005/diff/1/ui/base/cocoa/appkit_utils.mm
File ui/base/cocoa/appkit_utils.mm (right):

https://codereview.chromium.org/1588073005/diff/1/ui/base/cocoa/appkit_utils....
ui/base/cocoa/appkit_utils.mm:35: if (!base::mac::IsOSYosemiteOrLater()) {
On 2016/01/19 19:34:23, shrike wrote:
> On 2016/01/17 23:53:18, tapted wrote:
> > nit: IsOSMavericksOrEarlier() ?
> 
> I guess I like !IsOSYosemiteOrLater() because the code is saying for
everything
> before Yosemite, return false. Saying 
> 
>   if (base::mac::IsOSMavericksOrEarlier()) {
>     return false;
>   } else if (base::mac::IsOSYosemite()) {
>     return true;
>   }
> 
> means that everything after Yosemite, and in between Mavericks and Yosemite,
> falls through. I myself have a little trouble remembering the OSes in between
> MountainLion and Yosemite, so to me my version is more clear.

Makes sense, and I agree with your point. (and I'm truly not fussed, but an
option to get the same "everything else" logic could be to do the El Capitan
check first, similar to ShouldWindowsMiniaturize, then ShouldWindowsMaximize
could finish up as simply `return base::mac::IsOSYosemite();`)

[shrike, 2016-01-20 17:23:54]

https://codereview.chromium.org/1588073005/diff/1/ui/base/cocoa/appkit_utils.mm
File ui/base/cocoa/appkit_utils.mm (right):

https://codereview.chromium.org/1588073005/diff/1/ui/base/cocoa/appkit_utils....
ui/base/cocoa/appkit_utils.mm:35: if (!base::mac::IsOSYosemiteOrLater()) {
On 2016/01/19 22:16:34, tapted wrote:
> On 2016/01/19 19:34:23, shrike wrote:
> > On 2016/01/17 23:53:18, tapted wrote:
> > > nit: IsOSMavericksOrEarlier() ?
> > 
> > I guess I like !IsOSYosemiteOrLater() because the code is saying for
> everything
> > before Yosemite, return false. Saying 
> > 
> >   if (base::mac::IsOSMavericksOrEarlier()) {
> >     return false;
> >   } else if (base::mac::IsOSYosemite()) {
> >     return true;
> >   }
> > 
> > means that everything after Yosemite, and in between Mavericks and Yosemite,
> > falls through. I myself have a little trouble remembering the OSes in
between
> > MountainLion and Yosemite, so to me my version is more clear.
> 
> Makes sense, and I agree with your point. (and I'm truly not fussed, but an
> option to get the same "everything else" logic could be to do the El Capitan
> check first, similar to ShouldWindowsMiniaturize, then ShouldWindowsMaximize
> could finish up as simply `return base::mac::IsOSYosemite();`)

Yeah, that makes a lot of sense, and has nice symmetry.

I thought about your comment on making a single preferences read. Originally I
half believed that these ShouldMin/Max functions were needed elsewhere but given
that they both are only used once in AppkitUtils I took a stab at combining
them. The logic is a little hairier than before but it's basically the same: at
the start I do the special El Capitan work, and then I check ShouldMinimize and
then ShouldMaximize, just as WindowTitlebarReceivedDoubleClick() did before.

PTAL

[tapted, 2016-01-21 00:27:20]

You are far too diligent :). It looks nice! And I feel terrible for all the
timezone latency, but I have one more suggestion. lgtm after that.

https://codereview.chromium.org/1588073005/diff/40001/ui/base/cocoa/appkit_ut...
File ui/base/cocoa/appkit_utils.mm (right):

https://codereview.chromium.org/1588073005/diff/40001/ui/base/cocoa/appkit_ut...
ui/base/cocoa/appkit_utils.mm:19: const int kNoAction = 0;
enum class DoubleClickAction {
  NONE,
  MINIMIZE,
  MAXIMIZE,
};

? It's a bit more verbose, but it feels unusual to use integer constants for
this :/

Then the switch can have a `case DoubleClickAction::NONE:` instead of a
`default` case (the advantage - [in general - it's a stretch to apply it here]
would be that adding a possible action to the enum would result in compile
errors in any switch statement that didn't handle it).

[shrike, 2016-01-21 00:52:50]

No worries on the time zone difference. This is kind of a casual cl so no real
rush on getting it out.

PTAL

https://codereview.chromium.org/1588073005/diff/40001/ui/base/cocoa/appkit_ut...
File ui/base/cocoa/appkit_utils.mm (right):

https://codereview.chromium.org/1588073005/diff/40001/ui/base/cocoa/appkit_ut...
ui/base/cocoa/appkit_utils.mm:19: const int kNoAction = 0;
On 2016/01/21 00:27:20, tapted wrote:
> enum class DoubleClickAction {
>   NONE,
>   MINIMIZE,
>   MAXIMIZE,
> };
> 
> ? It's a bit more verbose, but it feels unusual to use integer constants for
> this :/
> 
> Then the switch can have a `case DoubleClickAction::NONE:` instead of a
> `default` case (the advantage - [in general - it's a stretch to apply it here]
> would be that adding a possible action to the enum would result in compile
> errors in any switch statement that didn't handle it).

This is great, actually. I wasn't sure of the right way to declare these
symbols. I like that it means generating an error in the switch statement if
more items get added to the enum, and in my own code I prefer labels like
DoubleClickActionNone, DoubleClickActionMinimize - that's not the convention in
Chrome, but declaring the enum this way gets me the equivalent.

[tapted, 2016-01-21 00:54:15]

sweet! lgtm

[shrike, 2016-01-21 16:31:03]

The CQ bit was checked by shrike@chromium.org

[commit-bot: I haz the power, 2016-01-21 16:32:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1588073005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1588073005/60001

[commit-bot: I haz the power, 2016-01-21 17:45:42]

Description was changed from

==========
Follow El Capitan Dock settings for double-click in window title bar.

In El Capitan there are Dock settings governing what happens when you
double-click in a window's title. This change allows browser windows
to behave like other windows on the system that respect this setting.

BUG=576520
==========

to

==========
Follow El Capitan Dock settings for double-click in window title bar.

In El Capitan there are Dock settings governing what happens when you
double-click in a window's title. This change allows browser windows
to behave like other windows on the system that respect this setting.

BUG=576520
==========

[commit-bot: I haz the power, 2016-01-21 17:45:43]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-21 17:47:00]

Description was changed from

==========
Follow El Capitan Dock settings for double-click in window title bar.

In El Capitan there are Dock settings governing what happens when you
double-click in a window's title. This change allows browser windows
to behave like other windows on the system that respect this setting.

BUG=576520
==========

to

==========
Follow El Capitan Dock settings for double-click in window title bar.

In El Capitan there are Dock settings governing what happens when you
double-click in a window's title. This change allows browser windows
to behave like other windows on the system that respect this setting.

BUG=576520

Committed: https://crrev.com/459a5b538a486568bda23d098aec5a430c6ba1fe
Cr-Commit-Position: refs/heads/master@{#370726}
==========

[commit-bot: I haz the power, 2016-01-21 17:47:01]

Patchset 4 (id:??) landed as
https://crrev.com/459a5b538a486568bda23d098aec5a430c6ba1fe
Cr-Commit-Position: refs/heads/master@{#370726}