Fix for Exit Fullscreen Animation

Fix for the exit fullscreen animation so that it is more smooth in 10.10+.
This CL changes the approach for exiting fullscreen animation so that
resizes the content view instead of the layer. As a result, some of the old
hacks are also removed in this CL.

In addition, the fullscreen animation's duration is shortened to create a faster
transition.

BUG=530801, 549063
Committed: https://crrev.com/207a1ee1b1715f34d5435b604e287cf4f816a8e7
Cr-Commit-Position: refs/heads/master@{#358906}

[spqchan, 2015-11-06 01:46:51]

Description was changed from

==========
Exit Fullscreen transition fix

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

to

==========
Fix for the exit fullscreen animation so that it is more smooth in 10.10+.
This approach resizes the content view instead of the layer.


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

[spqchan, 2015-11-06 21:30:24]

Description was changed from

==========
Fix for the exit fullscreen animation so that it is more smooth in 10.10+.
This approach resizes the content view instead of the layer.


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

to

==========
Fix for the exit fullscreen animation so that it is more smooth in 10.10+.
This approach resizes the content view instead of the layer.


BUG=530801
==========

[spqchan, 2015-11-07 02:46:03]

Description was changed from

==========
Fix for the exit fullscreen animation so that it is more smooth in 10.10+.
This approach resizes the content view instead of the layer.


BUG=530801
==========

to

==========
Fix for the exit fullscreen animation so that it is more smooth in 10.10+.
This CL changes the approach for exiting fullscreen animation so that
resizes the content view instead of the layer.

In addition, the fullscreen animation's duration is halved to create a faster
transition.

BUG=530801
==========

[spqchan, 2015-11-07 03:01:42]

Description was changed from

==========
Fix for the exit fullscreen animation so that it is more smooth in 10.10+.
This CL changes the approach for exiting fullscreen animation so that
resizes the content view instead of the layer.

In addition, the fullscreen animation's duration is halved to create a faster
transition.

BUG=530801
==========

to

==========
Fix for the exit fullscreen animation so that it is more smooth in 10.10+.
This CL changes the approach for exiting fullscreen animation so that
resizes the content view instead of the layer. As a result, some of the old
hacks are also removed in this CL.

In addition, the fullscreen animation's duration is shortened create a faster
transition. 

BUG=530801
==========

[spqchan, 2015-11-07 03:09:53]

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

[spqchan, 2015-11-07 03:09:53]

PTAL

[erikchen, 2015-11-09 18:29:21]

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:1116: if
(base::mac::IsOSMavericks() && !enterFullScreen)
Why? Add a link to a crbug post or add more details here.

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm (right):

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:25: CGFloat
const kAnimationDurationFrame = 0.5;
maybe kAnimationDurationFraction?

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:297:
[snapshotWindow_ orderFront:nil];
what's with the different in timing? Can you add comments indicating why this
happens at a different time from line 270?

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:303: // must
transparent. This is a hack that forces the layer to remove
s/must/must be/

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:305: [rootView
setWantsLayer:NO];
Why does this hack work? Shouldn't we be able to make the window's background be
clearColor without this hack?

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:327:
[contentView setAutoresizesSubviews:NO];
Why? If we're preventing the frame from changing, this should be unnecessary,
right?

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:474:
setStyleMask:[primaryWindow_ styleMask] & ~NSFullScreenWindowMask];
does this need to be 2 separate calls to setStyleMask?

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/framed_browser_window.mm (right):

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/framed_browser_window.mm:107: styleMaskLock_ = NO;
unnecessary.

[spqchan, 2015-11-09 22:27:19]

PTAL

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:1116: if
(base::mac::IsOSMavericks() && !enterFullScreen)
On 2015/11/09 18:29:21, erikchen wrote:
> Why? Add a link to a crbug post or add more details here.

Done.

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm (right):

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:25: CGFloat
const kAnimationDurationFrame = 0.5;
On 2015/11/09 18:29:21, erikchen wrote:
> maybe kAnimationDurationFraction?

Done.

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:297:
[snapshotWindow_ orderFront:nil];
On 2015/11/09 18:29:21, erikchen wrote:
> what's with the different in timing? Can you add comments indicating why this
> happens at a different time from line 270?

Done.

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:303: // must
transparent. This is a hack that forces the layer to remove
On 2015/11/09 18:29:21, erikchen wrote:
> s/must/must be/

Done.

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:305: [rootView
setWantsLayer:NO];
On 2015/11/09 18:29:21, erikchen wrote:
> Why does this hack work? Shouldn't we be able to make the window's background
be
> clearColor without this hack?

Having NSTexturedBackgroundWindowMask prevents the window's background from
being changed.
Simply removing the mask and setting the clearColor does not work, there are
certain parts of the CALayer that's not being updated causing it to think it
still have a textured background, thus ignoring the window's new background
color. This hack forces the layer to be updated with the new background color.

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:327:
[contentView setAutoresizesSubviews:NO];
On 2015/11/09 18:29:21, erikchen wrote:
> Why? If we're preventing the frame from changing, this should be unnecessary,
> right?

We don't actually prevent the frame from changing. We prevent the size of the
content view from becoming smaller than the window's frame. 

In OSX 10.11, when the NSFullScreenWindowMask is added or removed, window's
frame changes slightly which causes a janky movement at the beginning/end of the
animation. Anyway I added a comment for this.

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:474:
setStyleMask:[primaryWindow_ styleMask] & ~NSFullScreenWindowMask];
On 2015/11/09 18:29:21, erikchen wrote:
> does this need to be 2 separate calls to setStyleMask?

I separated them so that it looks cleaner. I'll go ahead and combine them
anyway.

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/framed_browser_window.mm (right):

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/framed_browser_window.mm:107: styleMaskLock_ = NO;
On 2015/11/09 18:29:21, erikchen wrote:
> unnecessary.

Done.

[spqchan, 2015-11-09 22:34:30]

Description was changed from

==========
Fix for the exit fullscreen animation so that it is more smooth in 10.10+.
This CL changes the approach for exiting fullscreen animation so that
resizes the content view instead of the layer. As a result, some of the old
hacks are also removed in this CL.

In addition, the fullscreen animation's duration is shortened create a faster
transition. 

BUG=530801
==========

to

==========
Fix for the exit fullscreen animation so that it is more smooth in 10.10+.
This CL changes the approach for exiting fullscreen animation so that
resizes the content view instead of the layer. As a result, some of the old
hacks are also removed in this CL.

In addition, the fullscreen animation's duration is shortened create a faster
transition. 

BUG=530801,549063
==========

[erikchen, 2015-11-10 02:15:44]

lgtm with nits

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:1116: if
(base::mac::IsOSMavericks() && !enterFullScreen)
On 2015/11/09 22:27:19, spqchan wrote:
> On 2015/11/09 18:29:21, erikchen wrote:
> > Why? Add a link to a crbug post or add more details here.
> 
> Done.


Use the link: 
https://code.google.com/p/chromium/issues/detail?id=526327#c3

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm (right):

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:305: [rootView
setWantsLayer:NO];
On 2015/11/09 22:27:19, spqchan wrote:
> On 2015/11/09 18:29:21, erikchen wrote:
> > Why does this hack work? Shouldn't we be able to make the window's
background
> be
> > clearColor without this hack?
> 
> Having NSTexturedBackgroundWindowMask prevents the window's background from
> being changed.
> Simply removing the mask and setting the clearColor does not work, there are
> certain parts of the CALayer that's not being updated causing it to think it
> still have a textured background, thus ignoring the window's new background
> color. This hack forces the layer to be updated with the new background color.

Why can't you use -setNeedsDisplay or something similar directly on the layer?

[spqchan, 2015-11-10 18:45:31]

PTAL

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:1116: if
(base::mac::IsOSMavericks() && !enterFullScreen)
On 2015/11/10 02:15:43, erikchen wrote:
> On 2015/11/09 22:27:19, spqchan wrote:
> > On 2015/11/09 18:29:21, erikchen wrote:
> > > Why? Add a link to a crbug post or add more details here.
> > 
> > Done.
> 
> 
> Use the link: 
> https://code.google.com/p/chromium/issues/detail?id=526327#c3

Done.

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm (right):

https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:305: [rootView
setWantsLayer:NO];
On 2015/11/10 02:15:43, erikchen wrote:
> On 2015/11/09 22:27:19, spqchan wrote:
> > On 2015/11/09 18:29:21, erikchen wrote:
> > > Why does this hack work? Shouldn't we be able to make the window's
> background
> > be
> > > clearColor without this hack?
> > 
> > Having NSTexturedBackgroundWindowMask prevents the window's background from
> > being changed.
> > Simply removing the mask and setting the clearColor does not work, there are
> > certain parts of the CALayer that's not being updated causing it to think it
> > still have a textured background, thus ignoring the window's new background
> > color. This hack forces the layer to be updated with the new background
color.
> 
> Why can't you use -setNeedsDisplay or something similar directly on the layer?


I tried to use setNeedsDisplay, updateLayer and various undocumented layer
"redrawing/updating" methods I found via Hopper.
I strongly suspect that the issue isn't that the layer isn't updating its
appearance, but rather some undocumented variables associated with the layer are
not being set.

[erikchen, 2015-11-10 18:48:02]

On 2015/11/10 18:45:31, spqchan wrote:
> PTAL
> 
>
https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
> File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):
> 
>
https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
> chrome/browser/ui/cocoa/browser_window_controller_private.mm:1116: if
> (base::mac::IsOSMavericks() && !enterFullScreen)
> On 2015/11/10 02:15:43, erikchen wrote:
> > On 2015/11/09 22:27:19, spqchan wrote:
> > > On 2015/11/09 18:29:21, erikchen wrote:
> > > > Why? Add a link to a crbug post or add more details here.
> > > 
> > > Done.
> > 
> > 
> > Use the link: 
> > https://code.google.com/p/chromium/issues/detail?id=526327#c3
> 
> Done.
> 
>
https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
> File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm (right):
> 
>
https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
> chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:305: [rootView
> setWantsLayer:NO];
> On 2015/11/10 02:15:43, erikchen wrote:
> > On 2015/11/09 22:27:19, spqchan wrote:
> > > On 2015/11/09 18:29:21, erikchen wrote:
> > > > Why does this hack work? Shouldn't we be able to make the window's
> > background
> > > be
> > > > clearColor without this hack?
> > > 
> > > Having NSTexturedBackgroundWindowMask prevents the window's background
from
> > > being changed.
> > > Simply removing the mask and setting the clearColor does not work, there
are
> > > certain parts of the CALayer that's not being updated causing it to think
it
> > > still have a textured background, thus ignoring the window's new
background
> > > color. This hack forces the layer to be updated with the new background
> color.
> > 
> > Why can't you use -setNeedsDisplay or something similar directly on the
layer?
> 
> 
> I tried to use setNeedsDisplay, updateLayer and various undocumented layer
> "redrawing/updating" methods I found via Hopper.
> I strongly suspect that the issue isn't that the layer isn't updating its
> appearance, but rather some undocumented variables associated with the layer
are
> not being set.

okay, still lgtm

[erikchen, 2015-11-10 18:48:35]

On 2015/11/10 18:48:02, erikchen wrote:
> On 2015/11/10 18:45:31, spqchan wrote:
> > PTAL
> > 
> >
>
https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
> > File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):
> > 
> >
>
https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
> > chrome/browser/ui/cocoa/browser_window_controller_private.mm:1116: if
> > (base::mac::IsOSMavericks() && !enterFullScreen)
> > On 2015/11/10 02:15:43, erikchen wrote:
> > > On 2015/11/09 22:27:19, spqchan wrote:
> > > > On 2015/11/09 18:29:21, erikchen wrote:
> > > > > Why? Add a link to a crbug post or add more details here.
> > > > 
> > > > Done.
> > > 
> > > 
> > > Use the link: 
> > > https://code.google.com/p/chromium/issues/detail?id=526327#c3
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
> > File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm
(right):
> > 
> >
>
https://codereview.chromium.org/1408033010/diff/100001/chrome/browser/ui/coco...
> > chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:305:
[rootView
> > setWantsLayer:NO];
> > On 2015/11/10 02:15:43, erikchen wrote:
> > > On 2015/11/09 22:27:19, spqchan wrote:
> > > > On 2015/11/09 18:29:21, erikchen wrote:
> > > > > Why does this hack work? Shouldn't we be able to make the window's
> > > background
> > > > be
> > > > > clearColor without this hack?
> > > > 
> > > > Having NSTexturedBackgroundWindowMask prevents the window's background
> from
> > > > being changed.
> > > > Simply removing the mask and setting the clearColor does not work, there
> are
> > > > certain parts of the CALayer that's not being updated causing it to
think
> it
> > > > still have a textured background, thus ignoring the window's new
> background
> > > > color. This hack forces the layer to be updated with the new background
> > color.
> > > 
> > > Why can't you use -setNeedsDisplay or something similar directly on the
> layer?
> > 
> > 
> > I tried to use setNeedsDisplay, updateLayer and various undocumented layer
> > "redrawing/updating" methods I found via Hopper.
> > I strongly suspect that the issue isn't that the layer isn't updating its
> > appearance, but rather some undocumented variables associated with the layer
> are
> > not being set.
> 
> okay, still lgtm

nit on cl description: s/is shortened create/is shortened to create

[spqchan, 2015-11-10 19:30:24]

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-10 19:34:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408033010/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408033010/140001

[commit-bot: I haz the power, 2015-11-10 20:05:32]

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

[commit-bot: I haz the power, 2015-11-10 20:05:33]

Dry run: This issue passed the CQ dry run.

[spqchan, 2015-11-10 21:19:33]

Description was changed from

==========
Fix for the exit fullscreen animation so that it is more smooth in 10.10+.
This CL changes the approach for exiting fullscreen animation so that
resizes the content view instead of the layer. As a result, some of the old
hacks are also removed in this CL.

In addition, the fullscreen animation's duration is shortened create a faster
transition. 

BUG=530801,549063
==========

to

==========
Fix for the exit fullscreen animation so that it is more smooth in 10.10+.
This CL changes the approach for exiting fullscreen animation so that
resizes the content view instead of the layer. As a result, some of the old
hacks are also removed in this CL.

In addition, the fullscreen animation's duration is shortened to create a faster
transition. 

BUG=530801,549063
==========

[spqchan, 2015-11-10 21:19:53]

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

[spqchan, 2015-11-10 21:19:54]

+avi for LGTM stamp

[Avi (use Gerrit), 2015-11-10 21:23:23]

lgtm

stampity stamp

[spqchan1, 2015-11-10 21:24:11]

The CQ bit was checked by spqchan@google.com

[commit-bot: I haz the power, 2015-11-10 21:24:56]

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

[commit-bot: I haz the power, 2015-11-10 21:30:54]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-11-10 21:31:43]

Patchset 8 (id:??) landed as
https://crrev.com/207a1ee1b1715f34d5435b604e287cf4f816a8e7
Cr-Commit-Position: refs/heads/master@{#358906}