[Mac] Fix for fullscreen toolbar

[Mac] Fix for fullscreen toolbar

Added back mouse tracking to fix an edge case where the menu
bar disappears when but the mouse is still hovering the toolbar.

The tracking area is only used to keep the toolbar active when this
happens. The toolbar will hide when the mouse leaves the tracking
area.

BUG=639562
Committed: https://crrev.com/e776f9c0602bafaf7ec33d022f543e86c4191a8f
Cr-Commit-Position: refs/heads/master@{#414888}

[spqchan, 2016-08-23 18:43:40]

Description was changed from

==========
Revert "[Mac] Reworked FullscreenToolbarController"

This reverts commit cccd612ee508dd2ee75f5d70e46b33bb17a7b88f.

BUG=
==========

to

==========
[Mac] Reworked FullscreenToolbarController

(first patch is the revert fyi)
==========

[erikchen, 2016-08-23 21:19:57]

erikchen@chromium.org changed reviewers:
+ erikchen@chromium.org

[erikchen, 2016-08-23 21:19:58]

https://codereview.chromium.org/2272783002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (right):

https://codereview.chromium.org/2272783002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:341:
[browserController_ isBarVisibilityLockedForOwner:nil] ? 1.0 : fraction;
This line concerns me a lot. The previous method seemed very straight forward:

1. Change toolbarFraction to |fraction|.
2. Redraw.

The new method now does:

1. Change toolbarFraction to |fraction|, unless some external state has been
set, in which case set it to 1.0.
2. Redraw.

This seems like we're mixing state and layout logic again. If at some point the
state for "[browserController_ isBarVisibilityLockedForOwner:nil]" changes,
won't toolbarFraction_ have the wrong information in it?

https://codereview.chromium.org/2272783002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:418: if (![self
isMouseOnScreen] && progress > menubarFraction_)
If the active screen changes, but the menubar fraction doesn't change, then the
|menubarFraction_| ivar will have the wrong information, and problems will
ensue, I believe? Layout logic should be kept separate from underlying state.

[spqchan, 2016-08-24 23:19:41]

Okay, I moved some stuff around so hopefully this is better now.

Apparently there was a bit of toolbarFraction logic in BrowserWindowLayout. Got
rid of that.

I got rid of toolbarFraction_ and replaced it with
toolbarFractionFromMenuProgress_, which would only be set by the
menubarProgress.

[erikchen, 2016-08-24 23:52:59]

This PS was much easier to review. Thanks!

https://codereview.chromium.org/2272783002/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (right):

https://codereview.chromium.org/2272783002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:292: -
(void)revealToolbarForTabStripChanges {
nit: Having a method named "foo" which changes the value of an ivar named "foo_"
is an antipattern. "foo" should be a getter, and "setFoo:" should be the setter.
Read more about this here:
https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Coding...

Based on my understanding of the use of this ivar, I recommend:
keep the name of the method -revealToolbarForTabStripChanges.
Change the ivar to be called isRevealingToolbarForTabStripChanges_.

https://codereview.chromium.org/2272783002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:300: if ([self
toolbarFraction] == 0.0) {
nit: You have a lot of double comparisons in your CL. They happen to work
because 0.0 and 1.0 have exact representations in IEEE754. That being said, an
implicit casts between double and floats will cause problems:
http://stackoverflow.com/questions/9834273/floating-point-comparison-of-const...

This works for now because CGFloat is a double on 64-bit platforms. However,
this is liable to break in the future when someone accidentally introduces a
cast. Can we find a way to avoid double comparisons altogether?

https://codereview.chromium.org/2272783002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:326: if ([event
trackingArea] == trackingArea_.get()) {
nit: Should this be DCHECK_EQ([event trackingArea], trackingArea_.get())

https://codereview.chromium.org/2272783002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:363: -
(CGFloat)toolbarFraction {
Wow. This is amazing. Thank you.

https://codereview.chromium.org/2272783002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:405:
currentAnimation_.reset();
If -toolbarFraction is called after the animation has stopped, but while the
hideTimer_ is still running, shouldn't it also return 1.0?

https://codereview.chromium.org/2272783002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:437: if
(menubarFraction_ == 1.0)
This double comparison relies on the assumption that the CGFloat we get from
AppKit has the double literal 1.0, which is not guaranteed. 

You probably want to use std::numeric_limits<CGFloat>::epsilon

http://stackoverflow.com/questions/4548004/how-to-correctly-and-standardly-co...

https://codereview.chromium.org/2272783002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:441: // is still
interacting with the. If it is, then don't set
incomplete sentence?

https://codereview.chromium.org/2272783002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:447:
toolbarFractionFromMenuProgress_ = progress;
Can we separate the logic for toolbarFractionFromMenuProgress_ from the
underlying state?

Can we instead have toolbarFractionFromMenuProgress_ be a calculated property
based on:

1) menuProgress_
2) isMenuBarDisappearing_

Both (1) and (2) would be recalculated each time setMenuBarRevealProgress: is
called.

[spqchan, 2016-08-26 00:46:21]

Description was changed from

==========
[Mac] Reworked FullscreenToolbarController

(first patch is the revert fyi)
==========

to

==========
[Mac] Fix for fullscreen toolbar

Added back mouse tracking to fix an edge case where the menu
bar disappears when but the mouse is still hovering the toolbar.

The tracking area is only used to keep the toolbar active when this
happens. The toolbar will hide when the mouse leaves the tracking
area.

BUG=639562
==========

[spqchan, 2016-08-26 00:46:30]

PTAL

https://codereview.chromium.org/2272783002/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (right):

https://codereview.chromium.org/2272783002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:292: -
(void)revealToolbarForTabStripChanges {
On 2016/08/24 23:52:58, erikchen wrote:
> nit: Having a method named "foo" which changes the value of an ivar named
"foo_"
> is an antipattern. "foo" should be a getter, and "setFoo:" should be the
setter.
> Read more about this here:
>
https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Coding...
> 
> Based on my understanding of the use of this ivar, I recommend:
> keep the name of the method -revealToolbarForTabStripChanges.
> Change the ivar to be called isRevealingToolbarForTabStripChanges_.

Done.

https://codereview.chromium.org/2272783002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:300: if ([self
toolbarFraction] == 0.0) {
On 2016/08/24 23:52:58, erikchen wrote:
> nit: You have a lot of double comparisons in your CL. They happen to work
> because 0.0 and 1.0 have exact representations in IEEE754. That being said, an
> implicit casts between double and floats will cause problems:
>
http://stackoverflow.com/questions/9834273/floating-point-comparison-of-const...
> 
> This works for now because CGFloat is a double on 64-bit platforms. However,
> this is liable to break in the future when someone accidentally introduces a
> cast. Can we find a way to avoid double comparisons altogether?

Added IsCGFloatEqual(). Is this what you mean?

https://codereview.chromium.org/2272783002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:326: if ([event
trackingArea] == trackingArea_.get()) {
On 2016/08/24 23:52:58, erikchen wrote:
> nit: Should this be DCHECK_EQ([event trackingArea], trackingArea_.get())

Done.

https://codereview.chromium.org/2272783002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:363: -
(CGFloat)toolbarFraction {
On 2016/08/24 23:52:58, erikchen wrote:
> Wow. This is amazing. Thank you.

Ack

https://codereview.chromium.org/2272783002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:405:
currentAnimation_.reset();
On 2016/08/24 23:52:58, erikchen wrote:
> If -toolbarFraction is called after the animation has stopped, but while the
> hideTimer_ is still running, shouldn't it also return 1.0?

Good point.

https://codereview.chromium.org/2272783002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:437: if
(menubarFraction_ == 1.0)
On 2016/08/24 23:52:58, erikchen wrote:
> This double comparison relies on the assumption that the CGFloat we get from
> AppKit has the double literal 1.0, which is not guaranteed. 
> 
> You probably want to use std::numeric_limits<CGFloat>::epsilon
> 
>
http://stackoverflow.com/questions/4548004/how-to-correctly-and-standardly-co...

Ditto from above

https://codereview.chromium.org/2272783002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:441: // is still
interacting with the. If it is, then don't set
On 2016/08/24 23:52:58, erikchen wrote:
> incomplete sentence?

Done.

https://codereview.chromium.org/2272783002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:447:
toolbarFractionFromMenuProgress_ = progress;
On 2016/08/24 23:52:58, erikchen wrote:
> Can we separate the logic for toolbarFractionFromMenuProgress_ from the
> underlying state?
> 
> Can we instead have toolbarFractionFromMenuProgress_ be a calculated property
> based on:
> 
> 1) menuProgress_
> 2) isMenuBarDisappearing_
> 
> Both (1) and (2) would be recalculated each time setMenuBarRevealProgress: is
> called.

I already tried that. I find that having |toolbarFractionFromMenuProgress_| is
simpler and easier to understand, because there's a couple of edge cases we'll
have to cover.

[erikchen, 2016-08-26 00:56:59]

https://codereview.chromium.org/2272783002/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (right):

https://codereview.chromium.org/2272783002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:300: if ([self
toolbarFraction] == 0.0) {
On 2016/08/26 00:46:30, spqchan wrote:
> On 2016/08/24 23:52:58, erikchen wrote:
> > nit: You have a lot of double comparisons in your CL. They happen to work
> > because 0.0 and 1.0 have exact representations in IEEE754. That being said,
an
> > implicit casts between double and floats will cause problems:
> >
>
http://stackoverflow.com/questions/9834273/floating-point-comparison-of-const...
> > 
> > This works for now because CGFloat is a double on 64-bit platforms. However,
> > this is liable to break in the future when someone accidentally introduces a
> > cast. Can we find a way to avoid double comparisons altogether?
> 
> Added IsCGFloatEqual(). Is this what you mean?

Ideally, we would be able to avoid any type of float comparison (even using an
epsilon). e.g., if we know the toolbar is showing, instead of relying on
fraction == 1.0, have an enum kToolbarStatus { Hidden, Showing, Animating}.

That being said, what you've done is enough to alleviate my concerns.

https://codereview.chromium.org/2272783002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:447:
toolbarFractionFromMenuProgress_ = progress;
On 2016/08/26 00:46:30, spqchan wrote:
> On 2016/08/24 23:52:58, erikchen wrote:
> > Can we separate the logic for toolbarFractionFromMenuProgress_ from the
> > underlying state?
> > 
> > Can we instead have toolbarFractionFromMenuProgress_ be a calculated
property
> > based on:
> > 
> > 1) menuProgress_
> > 2) isMenuBarDisappearing_
> > 
> > Both (1) and (2) would be recalculated each time setMenuBarRevealProgress:
is
> > called.
> 
> I already tried that. I find that having |toolbarFractionFromMenuProgress_| is
> simpler and easier to understand, because there's a couple of edge cases we'll
> have to cover.

Acknowledged.

https://codereview.chromium.org/2272783002/diff/120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (right):

https://codereview.chromium.org/2272783002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:430:
shouldAnimateToolbarOut_ = NO;
Does this need to get unset anytime currentAnimation_ is reset, such as line
298?

https://codereview.chromium.org/2272783002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:434: //  [self
updateToolbar];
why is this commented out?

https://codereview.chromium.org/2272783002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:588:
shouldAnimateToolbarOut_ = YES;
should this be set at line 563, to mirror the location where it gets unset?

[spqchan, 2016-08-26 01:23:21]

PTAL

https://codereview.chromium.org/2272783002/diff/120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (right):

https://codereview.chromium.org/2272783002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:430:
shouldAnimateToolbarOut_ = NO;
On 2016/08/26 00:56:59, erikchen wrote:
> Does this need to get unset anytime currentAnimation_ is reset, such as line
> 298?

See below

https://codereview.chromium.org/2272783002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:434: //  [self
updateToolbar];
On 2016/08/26 00:56:59, erikchen wrote:
> why is this commented out?

Done.

https://codereview.chromium.org/2272783002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:588:
shouldAnimateToolbarOut_ = YES;
On 2016/08/26 00:56:58, erikchen wrote:
> should this be set at line 563, to mirror the location where it gets unset?

Actually...now that you mention I think it makes more sense to just set it and
unset it in hideTimer.

[erikchen, 2016-08-26 01:27:26]

https://codereview.chromium.org/2272783002/diff/120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (right):

https://codereview.chromium.org/2272783002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:588:
shouldAnimateToolbarOut_ = YES;
On 2016/08/26 01:23:21, spqchan wrote:
> On 2016/08/26 00:56:58, erikchen wrote:
> > should this be set at line 563, to mirror the location where it gets unset?
> 
> Actually...now that you mention I think it makes more sense to just set it and
> unset it in hideTimer.

Since a currentAnimation_ exists, doesn't the conditional that uses this
variable never get hit when we call [self animateToolbarVisibility:NO]?

[spqchan, 2016-08-26 01:29:30]

On 2016/08/26 01:27:26, erikchen wrote:
>
https://codereview.chromium.org/2272783002/diff/120001/chrome/browser/ui/coco...
> File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (right):
> 
>
https://codereview.chromium.org/2272783002/diff/120001/chrome/browser/ui/coco...
> chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:588:
> shouldAnimateToolbarOut_ = YES;
> On 2016/08/26 01:23:21, spqchan wrote:
> > On 2016/08/26 00:56:58, erikchen wrote:
> > > should this be set at line 563, to mirror the location where it gets
unset?
> > 
> > Actually...now that you mention I think it makes more sense to just set it
and
> > unset it in hideTimer.
> 
> Since a currentAnimation_ exists, doesn't the conditional that uses this
> variable never get hit when we call [self animateToolbarVisibility:NO]?

No, it's used in the DropdownAnimation's initializer, when it queries the
current toolbar fraction to determine the starting fraction

[erikchen, 2016-08-26 01:36:42]

lgtm

[spqchan, 2016-08-26 01:43:51]

spqchan@chromium.org changed reviewers:
+ rsesek@chromium.org

[spqchan, 2016-08-26 01:43:52]

thanks!

+rsesek for OWNERS stamp

[Robert Sesek, 2016-08-26 17:11:03]

lgtm

[spqchan, 2016-08-26 17:12:44]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2016-08-26 17:12:52]

On 2016/08/26 17:11:03, Robert Sesek wrote:
> lgtm

thanks!

[commit-bot: I haz the power, 2016-08-26 17:13:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 17:38:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-26 17:38:59]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[spqchan, 2016-08-26 18:00:25]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2016-08-26 18:00:25]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
erikchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2272783002/#ps160001
(title: "BrowserTest fix")

[commit-bot: I haz the power, 2016-08-26 18:01:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 19:12:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-26 19:12:47]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[spqchan, 2016-08-27 00:52:45]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2016-08-27 00:52:45]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
erikchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2272783002/#ps180001
(title: "Fixed browserwindowlayout unit_tests")

[commit-bot: I haz the power, 2016-08-27 00:53:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-27 10:30:16]

Description was changed from

==========
[Mac] Fix for fullscreen toolbar

Added back mouse tracking to fix an edge case where the menu
bar disappears when but the mouse is still hovering the toolbar.

The tracking area is only used to keep the toolbar active when this
happens. The toolbar will hide when the mouse leaves the tracking
area.

BUG=639562
==========

to

==========
[Mac] Fix for fullscreen toolbar

Added back mouse tracking to fix an edge case where the menu
bar disappears when but the mouse is still hovering the toolbar.

The tracking area is only used to keep the toolbar active when this
happens. The toolbar will hide when the mouse leaves the tracking
area.

BUG=639562
==========

[commit-bot: I haz the power, 2016-08-27 10:30:17]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-08-27 10:33:37]

Description was changed from

==========
[Mac] Fix for fullscreen toolbar

Added back mouse tracking to fix an edge case where the menu
bar disappears when but the mouse is still hovering the toolbar.

The tracking area is only used to keep the toolbar active when this
happens. The toolbar will hide when the mouse leaves the tracking
area.

BUG=639562
==========

to

==========
[Mac] Fix for fullscreen toolbar

Added back mouse tracking to fix an edge case where the menu
bar disappears when but the mouse is still hovering the toolbar.

The tracking area is only used to keep the toolbar active when this
happens. The toolbar will hide when the mouse leaves the tracking
area.

BUG=639562

Committed: https://crrev.com/e776f9c0602bafaf7ec33d022f543e86c4191a8f
Cr-Commit-Position: refs/heads/master@{#414888}
==========

[commit-bot: I haz the power, 2016-08-27 10:33:38]

Patchset 10 (id:??) landed as
https://crrev.com/e776f9c0602bafaf7ec33d022f543e86c4191a8f
Cr-Commit-Position: refs/heads/master@{#414888}