mac, fullscreen: Several bug fixes after major refactor.

mac, fullscreen: Several bug fixes after major refactor.

Bugs:
- Artifacts while entering AppKit Fullscreen in Yosemite.
- Presentation Mode and Canonical Fullscreen tab strip layout is incorrect when
menubar is hidden. (bug was already present before refactor)
- Canonical Fullscreen should ignore the mouse tracking area.

Non-trivial refactor:
- Previously, the layout of the tabstrip/omnibox in presentation mode was
queued off of a single property, a fraction that ranged from 0 to 1. It was
used both to reflect layout changes from the menu bar, and to reflect layout
changes from showing the tabstrip/omnibox. This caused problems for Canonical
Fullscreen, and also caused layout bugs when the menu bar is hidden. I created
2 properties: 1 to track offset of the tabstrip/omnibox, and another to track
offset of the menu bar.
  - I reintroduced the logic from fullscreen_mode_controller that uses Carbon
  APIs to track menu bar animations.
  - The jankiness during menu bar animations is because the web contents is
  being resized every step of the animation, which is slow.

Trivial refactor:
- Moved the "sliding style" enum and property from browser_window_controller
into presentation_mode_controller.
- Moved the "floatingBarShownFraction" property from browser_window_controller
into presentation_mode_controller, and renamed to "toolbarPercentage".

BUG=NONE

[erikchen, 2014-08-29 19:03:28]

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

[erikchen, 2014-08-29 19:03:29]

rsesek: Please review.

This CL depends on "mac: Major fullscreen refactor."
https://codereview.chromium.org/493143004/

[Robert Sesek, 2014-08-29 20:14:35]

Before we do this, can you rip out simplified fullscreen?

[erikchen, 2014-08-29 20:34:17]

I see this CL as being logically connected to the fullscreen refactor. I was
considering merging them together into a mega CL to more easily handle merges in
M38.

This CL is almost entirely orthogonal from fullscreen. Any chance you'd be
willing to review prior to axing simplified fullscreen?

[erikchen, 2014-08-29 23:20:36]

rsesek: For reference, the CL to remove simplified fullscreen is here:
https://codereview.chromium.org/523233002/

[erikchen, 2014-09-02 16:49:16]

rsesek: Once you review this CL, I'd like to merge this CL with the fullscreen
refactor CL and the removal of simplified fullscreen CL. The mega CL will have
been reviewed in parts, so I will TBR you. This allows for saner merges/reverts
on the release branches.

[Robert Sesek, 2014-09-02 19:59:49]

On 2014/09/02 16:49:16, erikchen wrote:
> rsesek: Once you review this CL, I'd like to merge this CL with the fullscreen
> refactor CL and the removal of simplified fullscreen CL. The mega CL will have
> been reviewed in parts, so I will TBR you. This allows for saner
merges/reverts
> on the release branches.

I'm not sure these should go to M38, which is already at beta: this will be a
large change that doesn't have a lot of test coverage. At the very least, these
need a week or two of testing on dev channel before merge.

https://codereview.chromium.org/523723002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

https://codereview.chromium.org/523723002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller.h:138: // where this is used
for a more detailed explanation.
Name the actual method so the reader does not have to go hunting for it.

https://codereview.chromium.org/523723002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/presentation_mode_controller.h (right):

https://codereview.chromium.org/523723002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/presentation_mode_controller.h:90: // The percentage of
the AppKit Menubar that is showing. Ranges from 0 to 1.
I think it's a bit unclear to call this a percentage, which is typically in the
range [0,100]. Fraction seems to be a better choice (and is what was used
before).

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

https://codereview.chromium.org/523723002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/presentation_mode_controller.mm:216: EventTypeSpec
eventSpec = {kEventClassMenu, 2004};
nit: I think style is for spaces inside {}

https://codereview.chromium.org/523723002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/presentation_mode_controller.mm:424: return
-std::floor((menubarPercentage_) * [self floatingBarVerticalOffset]);
Why parens around the first multiplicand?

[erikchen, 2014-09-02 20:43:21]

rsesek: PTAL

Re: merging to M38. This CL, as well as the fullscreen refactor, and the traffic
light additions, are required to make Canonical Fullscreen work in Yosemite. 

I agree that it would be wise to have more tests. The browser tests for
fullscreen have historically been very flaky (I believe they're almost all
disabled as this point?). Unit tests would be easier, but also less likely to
catch bugs. I'll write some sanity check unit tests for
presentation_mode_controller. Are there any tests in particular that you would
like to see?

As I mentioned in an earlier comment, I see this CL as the logical extension to
the fullscreen refactor, and I don't think it makes sense to merge them
separately. The CLs are separated to make reviewing them easier.

https://codereview.chromium.org/523723002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

https://codereview.chromium.org/523723002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller.h:138: // where this is used
for a more detailed explanation.
On 2014/09/02 19:59:48, rsesek wrote:
> Name the actual method so the reader does not have to go hunting for it.

Done.

https://codereview.chromium.org/523723002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/presentation_mode_controller.h (right):

https://codereview.chromium.org/523723002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/presentation_mode_controller.h:90: // The percentage of
the AppKit Menubar that is showing. Ranges from 0 to 1.
On 2014/09/02 19:59:48, rsesek wrote:
> I think it's a bit unclear to call this a percentage, which is typically in
the
> range [0,100]. Fraction seems to be a better choice (and is what was used
> before).

changed names to menubarFraction_ and toolbarFraction_

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

https://codereview.chromium.org/523723002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/presentation_mode_controller.mm:216: EventTypeSpec
eventSpec = {kEventClassMenu, 2004};
On 2014/09/02 19:59:48, rsesek wrote:
> nit: I think style is for spaces inside {}

This is the output of clang-format. According to the style guide, braced
initializer lists use function style spacing.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Braced_Initial...

https://codereview.chromium.org/523723002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/presentation_mode_controller.mm:424: return
-std::floor((menubarPercentage_) * [self floatingBarVerticalOffset]);
On 2014/09/02 19:59:48, rsesek wrote:
> Why parens around the first multiplicand?

removed.

[Robert Sesek, 2014-09-03 20:22:09]

https://codereview.chromium.org/523723002/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/presentation_mode_controller.h (right):

https://codereview.chromium.org/523723002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/presentation_mode_controller.h:103: 
nit: no blank line

https://codereview.chromium.org/523723002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/presentation_mode_controller.h:146: -
(void)setMenuBarRevealProgress:(CGFloat)progress;
Does this need to be public, or can this be moved into the .mm as a (Private)
category?

https://codereview.chromium.org/523723002/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/presentation_mode_controller.mm (right):

https://codereview.chromium.org/523723002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/presentation_mode_controller.mm:401: -
(void)changeToolbarPercentage:(CGFloat)fraction {
naming: Percentage -> Fraction

[Robert Sesek, 2014-09-03 20:29:04]

On 2014/09/02 20:43:21, erikchen wrote:
> rsesek: PTAL
> 
> Re: merging to M38. This CL, as well as the fullscreen refactor, and the
traffic
> light additions, are required to make Canonical Fullscreen work in Yosemite. 

I disagree with this statement. Fullscreen works, but the most-clear way to exit
it is no longer visible. There are still three ways to exit fullscreen, though:
menu item, keyboard shortcut, and the toolbar menu. You can certainly ask the
TPMs, but I'd rate a merged view of these changes as medium-to-high risk, with
comparatively little reward.

> I agree that it would be wise to have more tests. The browser tests for
> fullscreen have historically been very flaky (I believe they're almost all
> disabled as this point?). Unit tests would be easier, but also less likely to
> catch bugs. I'll write some sanity check unit tests for
> presentation_mode_controller. Are there any tests in particular that you would
> like to see?

Yes, the browser tests are busted and the unit tests don't exist. I think we
should try for more unit tests, though I imagine the setup may be quite
involved. The browser tests tend to flake out because of the AppKit interaction.
Maybe presentation mode is easier to test?

> As I mentioned in an earlier comment, I see this CL as the logical extension
to
> the fullscreen refactor, and I don't think it makes sense to merge them
> separately. The CLs are separated to make reviewing them easier.

I think the fact that a large refactor is coupled with the fixes is partly what
makes this change risky.

[erikchen, 2014-09-03 20:29:52]

rsesek: PTAL

https://codereview.chromium.org/523723002/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/presentation_mode_controller.h (right):

https://codereview.chromium.org/523723002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/presentation_mode_controller.h:103: 
On 2014/09/03 20:22:09, rsesek wrote:
> nit: no blank line

Done.

https://codereview.chromium.org/523723002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/presentation_mode_controller.h:146: -
(void)setMenuBarRevealProgress:(CGFloat)progress;
On 2014/09/03 20:22:09, rsesek wrote:
> Does this need to be public, or can this be moved into the .mm as a (Private)
> category?

Moved into private category.

https://codereview.chromium.org/523723002/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/presentation_mode_controller.mm (right):

https://codereview.chromium.org/523723002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/presentation_mode_controller.mm:401: -
(void)changeToolbarPercentage:(CGFloat)fraction {
On 2014/09/03 20:22:09, rsesek wrote:
> naming: Percentage -> Fraction

Done.

[Robert Sesek, 2014-09-03 20:34:45]

lgtm

[erikchen, 2014-09-03 20:34:52]

On 2014/09/03 20:29:04, rsesek wrote:
> On 2014/09/02 20:43:21, erikchen wrote:
> > rsesek: PTAL
> > 
> > Re: merging to M38. This CL, as well as the fullscreen refactor, and the
> traffic
> > light additions, are required to make Canonical Fullscreen work in Yosemite.

> 
> I disagree with this statement. Fullscreen works, but the most-clear way to
exit
> it is no longer visible. There are still three ways to exit fullscreen,
though:
> menu item, keyboard shortcut, and the toolbar menu. You can certainly ask the
> TPMs, but I'd rate a merged view of these changes as medium-to-high risk, with
> comparatively little reward.

Okay, I will talk to the TPM for M38.

> 
> > I agree that it would be wise to have more tests. The browser tests for
> > fullscreen have historically been very flaky (I believe they're almost all
> > disabled as this point?). Unit tests would be easier, but also less likely
to
> > catch bugs. I'll write some sanity check unit tests for
> > presentation_mode_controller. Are there any tests in particular that you
would
> > like to see?
> 
> Yes, the browser tests are busted and the unit tests don't exist. I think we
> should try for more unit tests, though I imagine the setup may be quite
> involved. The browser tests tend to flake out because of the AppKit
interaction.
> Maybe presentation mode is easier to test?

I will add more unit tests.

> 
> > As I mentioned in an earlier comment, I see this CL as the logical extension
> to
> > the fullscreen refactor, and I don't think it makes sense to merge them
> > separately. The CLs are separated to make reviewing them easier.
> 
> I think the fact that a large refactor is coupled with the fixes is partly
what
> makes this change risky.

Regardless of whether a merge to M38 happens, I want to merge this CL with the
refactor before landing on trunk. Do you have any objections to this?

[Robert Sesek, 2014-09-03 20:36:03]

On 2014/09/03 20:34:52, erikchen wrote:
> On 2014/09/03 20:29:04, rsesek wrote:
> > On 2014/09/02 20:43:21, erikchen wrote:
> > > rsesek: PTAL
> > > 
> > > Re: merging to M38. This CL, as well as the fullscreen refactor, and the
> > traffic
> > > light additions, are required to make Canonical Fullscreen work in
Yosemite.
> 
> > 
> > I disagree with this statement. Fullscreen works, but the most-clear way to
> exit
> > it is no longer visible. There are still three ways to exit fullscreen,
> though:
> > menu item, keyboard shortcut, and the toolbar menu. You can certainly ask
the
> > TPMs, but I'd rate a merged view of these changes as medium-to-high risk,
with
> > comparatively little reward.
> 
> Okay, I will talk to the TPM for M38.
> 
> > 
> > > I agree that it would be wise to have more tests. The browser tests for
> > > fullscreen have historically been very flaky (I believe they're almost all
> > > disabled as this point?). Unit tests would be easier, but also less likely
> to
> > > catch bugs. I'll write some sanity check unit tests for
> > > presentation_mode_controller. Are there any tests in particular that you
> would
> > > like to see?
> > 
> > Yes, the browser tests are busted and the unit tests don't exist. I think we
> > should try for more unit tests, though I imagine the setup may be quite
> > involved. The browser tests tend to flake out because of the AppKit
> interaction.
> > Maybe presentation mode is easier to test?
> 
> I will add more unit tests.

Great, thanks!

> 
> > 
> > > As I mentioned in an earlier comment, I see this CL as the logical
extension
> > to
> > > the fullscreen refactor, and I don't think it makes sense to merge them
> > > separately. The CLs are separated to make reviewing them easier.
> > 
> > I think the fact that a large refactor is coupled with the fixes is partly
> what
> > makes this change risky.
> 
> Regardless of whether a merge to M38 happens, I want to merge this CL with the
> refactor before landing on trunk. Do you have any objections to this?

No, definitely not. My concerns are only about M38 merging.

[erikchen, 2014-09-04 17:12:18]

This CL was merged into
https://codereview.chromium.org/533383002/