Polish Tabstrip and Overlay Issues in Fullscreen

Fixed a few fullscreen issues:
- Removed tab glimpse at the beginning of the animation by ensuring that
the content does not autoresize during the animation.
- Fixed an overlay sheet issue during the animation. If a sheet is opened
  during the animation, it will spuriously reposition and eventually move
  behind the window. This is fixed by hiding the sheet, disabling the
  repositioning during the transition, and then unhiding the sheet at the end
  of it.
- Along with the overlay sheet issue, a bug in the certificate viewer sheet is
  fixed. The code to set the overlay window's frame in |unhide| was
  outdated. The overlay window frame gets adjusted somewhere else so
  the code would just move the frame to an incorrect position.
BUG=558313, 556355
Committed: https://crrev.com/bcf58a50fc5e0f840130f3dcb45f36767d74d72b
Cr-Commit-Position: refs/heads/master@{#364228}

[spqchan, 2015-12-03 20:47:23]

Description was changed from

==========
Clean up


Fullscreen fixes

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

to

==========
Fixed a few fullscreen issues:
- Removed tab glimpse at the beginning of the animation by ensuring that 
the content does not autoresize during the animation.
- Fixed an overlay sheet issue during the animation. If a sheet is opened
  during the animation, it will spuriously reposition and eventually move
  behind the window. This is fixed by hiding the sheet, disabling the
  repositioning during the transition, and then unhiding the sheet at the end
  of it.
- Along with the overlay sheet issue, a bug in the certificate viewer sheet is
  fixed. The code to set the overlay window's frame in |unhide| was 
  outdated. The overlay window frame gets adjusted somewhere else so 
  the code would just move the frame to an incorrect position.
BUG=558313, 556355
==========

[spqchan, 2015-12-03 20:47:23]

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

[spqchan, 2015-12-03 20:48:49]

PTAL

[Robert Sesek, 2015-12-04 17:56:30]

Is it possible to write a browser test for this?

https://codereview.chromium.org/1495623008/diff/1/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_controller_private.h (right):

https://codereview.chromium.org/1495623008/diff/1/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller_private.h:183: // Hides or
unhides the sheet for fullscreen transition. The sheet should be
"the sheet" -> "any modal sheet" (or even more specific if possible)

https://codereview.chromium.org/1495623008/diff/1/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller_private.h:185: -
(void)hideSheetForFullscreenTransition:(BOOL)shoudHide;
What is |shouldHide| for? Maybe this should be
|setSheetHiddenForFullscreenTransition:| to make it more clear.

https://codereview.chromium.org/1495623008/diff/1/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/1495623008/diff/1/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:844:
ConstrainedWindowSheetController* sheet_controller =
naming: sheetController

https://codereview.chromium.org/1495623008/diff/1/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:847: if
(sheet_controller) {
You don't need this if, since you can nil-message ObjC objects.

https://codereview.chromium.org/1495623008/diff/1/chrome/browser/ui/cocoa/con...
File
chrome/browser/ui/cocoa/constrained_window/constrained_window_sheet_controller.mm
(right):

https://codereview.chromium.org/1495623008/diff/1/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/constrained_window/constrained_window_sheet_controller.mm:213:
[self showSheet:[sheetInfo sheet] forParentView:activeView_];
nit: needs {} since the condition is multi-line

[spqchan, 2015-12-07 18:27:52]

I can't write a test for the tabstrip, but I can look into writing one for the
sheet. 

Will a test that checks if the modal sheet is displayed after we enter/exit
fullscreen be sufficient?

https://codereview.chromium.org/1495623008/diff/1/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_controller_private.h (right):

https://codereview.chromium.org/1495623008/diff/1/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller_private.h:183: // Hides or
unhides the sheet for fullscreen transition. The sheet should be
On 2015/12/04 17:56:30, Robert Sesek wrote:
> "the sheet" -> "any modal sheet" (or even more specific if possible)

Done.

https://codereview.chromium.org/1495623008/diff/1/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller_private.h:185: -
(void)hideSheetForFullscreenTransition:(BOOL)shoudHide;
On 2015/12/04 17:56:30, Robert Sesek wrote:
> What is |shouldHide| for? Maybe this should be
> |setSheetHiddenForFullscreenTransition:| to make it more clear.

Done.

https://codereview.chromium.org/1495623008/diff/1/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/1495623008/diff/1/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:844:
ConstrainedWindowSheetController* sheet_controller =
On 2015/12/04 17:56:30, Robert Sesek wrote:
> naming: sheetController

Done.

https://codereview.chromium.org/1495623008/diff/1/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:847: if
(sheet_controller) {
On 2015/12/04 17:56:30, Robert Sesek wrote:
> You don't need this if, since you can nil-message ObjC objects.

Done.

https://codereview.chromium.org/1495623008/diff/1/chrome/browser/ui/cocoa/con...
File
chrome/browser/ui/cocoa/constrained_window/constrained_window_sheet_controller.mm
(right):

https://codereview.chromium.org/1495623008/diff/1/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/constrained_window/constrained_window_sheet_controller.mm:213:
[self showSheet:[sheetInfo sheet] forParentView:activeView_];
On 2015/12/04 17:56:30, Robert Sesek wrote:
> nit: needs {} since the condition is multi-line

Done.

[Robert Sesek, 2015-12-07 18:39:48]

On 2015/12/07 18:27:52, spqchan wrote:
> I can't write a test for the tabstrip, but I can look into writing one for the
> sheet. 
> 
> Will a test that checks if the modal sheet is displayed after we enter/exit
> fullscreen be sufficient?

Yes, that's exactly what I had in mind.

[spqchan, 2015-12-07 18:42:26]

On 2015/12/07 18:39:48, Robert Sesek wrote:
> On 2015/12/07 18:27:52, spqchan wrote:
> > I can't write a test for the tabstrip, but I can look into writing one for
the
> > sheet. 
> > 
> > Will a test that checks if the modal sheet is displayed after we enter/exit
> > fullscreen be sufficient?
> 
> Yes, that's exactly what I had in mind.

Awesome, I'll work on that and will notify you once that's ready.

[spqchan1, 2015-12-09 00:15:26]

spqchan@google.com changed reviewers:
+ spqchan@google.com

[spqchan1, 2015-12-09 00:15:26]

PTAL

Test added!

[Robert Sesek, 2015-12-09 16:29:05]

https://codereview.chromium.org/1495623008/diff/60001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac_browsertest.mm
(right):

https://codereview.chromium.org/1495623008/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac_browsertest.mm:173:
new FullscreenNotificationObserver());
This can probably be created on the stack ?

https://codereview.chromium.org/1495623008/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac_browsertest.mm:178:
waiter->Wait();
EXPECT that the alphaValue goes to 0 after the first one?

[spqchan, 2015-12-09 20:07:30]

PTAL

https://codereview.chromium.org/1495623008/diff/60001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac_browsertest.mm
(right):

https://codereview.chromium.org/1495623008/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac_browsertest.mm:173:
new FullscreenNotificationObserver());
On 2015/12/09 16:29:05, Robert Sesek wrote:
> This can probably be created on the stack ?


I tried it and it didn't work out.
I dug around and it looks like it will Wait once. After that, it will return
immediately:
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes...

https://codereview.chromium.org/1495623008/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac_browsertest.mm:178:
waiter->Wait();
On 2015/12/09 16:29:05, Robert Sesek wrote:
> EXPECT that the alphaValue goes to 0 after the first one?

No, since we are checking after it goes fullscreen, the sheet should be visible.
So it will be 1.0 for both.

[spqchan, 2015-12-09 20:07:41]

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

[commit-bot: I haz the power, 2015-12-09 20:11:03]

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

[commit-bot: I haz the power, 2015-12-09 20:11:04]

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

[commit-bot: I haz the power, 2015-12-09 20:11:04]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Robert Sesek, 2015-12-09 20:12:28]

LGTM

https://codereview.chromium.org/1495623008/diff/60001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac_browsertest.mm
(right):

https://codereview.chromium.org/1495623008/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/constrained_window/constrained_window_mac_browsertest.mm:178:
waiter->Wait();
On 2015/12/09 20:07:30, spqchan wrote:
> On 2015/12/09 16:29:05, Robert Sesek wrote:
> > EXPECT that the alphaValue goes to 0 after the first one?
> 
> No, since we are checking after it goes fullscreen, the sheet should be
visible.
> So it will be 1.0 for both.

Ah, right. I guess there isn't a good way to test that it gets hidden, then.

[spqchan1, 2015-12-09 20:13:22]

The CQ bit was checked by spqchan@google.com

[commit-bot: I haz the power, 2015-12-09 20:14:26]

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

[commit-bot: I haz the power, 2015-12-09 22:31:49]

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

[commit-bot: I haz the power, 2015-12-09 22:31:50]

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

[spqchan, 2015-12-09 23:17:00]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2015-12-09 23:17:00]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/1495623008/#ps80001
(title: "SSLCertificateViewerCocoaTest fixed")

[commit-bot: I haz the power, 2015-12-09 23:18:59]

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

[commit-bot: I haz the power, 2015-12-10 00:17:07]

Description was changed from

==========
Fixed a few fullscreen issues:
- Removed tab glimpse at the beginning of the animation by ensuring that 
the content does not autoresize during the animation.
- Fixed an overlay sheet issue during the animation. If a sheet is opened
  during the animation, it will spuriously reposition and eventually move
  behind the window. This is fixed by hiding the sheet, disabling the
  repositioning during the transition, and then unhiding the sheet at the end
  of it.
- Along with the overlay sheet issue, a bug in the certificate viewer sheet is
  fixed. The code to set the overlay window's frame in |unhide| was 
  outdated. The overlay window frame gets adjusted somewhere else so 
  the code would just move the frame to an incorrect position.
BUG=558313, 556355
==========

to

==========
Fixed a few fullscreen issues:
- Removed tab glimpse at the beginning of the animation by ensuring that 
the content does not autoresize during the animation.
- Fixed an overlay sheet issue during the animation. If a sheet is opened
  during the animation, it will spuriously reposition and eventually move
  behind the window. This is fixed by hiding the sheet, disabling the
  repositioning during the transition, and then unhiding the sheet at the end
  of it.
- Along with the overlay sheet issue, a bug in the certificate viewer sheet is
  fixed. The code to set the overlay window's frame in |unhide| was 
  outdated. The overlay window frame gets adjusted somewhere else so 
  the code would just move the frame to an incorrect position.
BUG=558313, 556355
==========

[commit-bot: I haz the power, 2015-12-10 00:17:07]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-10 00:18:24]

Description was changed from

==========
Fixed a few fullscreen issues:
- Removed tab glimpse at the beginning of the animation by ensuring that 
the content does not autoresize during the animation.
- Fixed an overlay sheet issue during the animation. If a sheet is opened
  during the animation, it will spuriously reposition and eventually move
  behind the window. This is fixed by hiding the sheet, disabling the
  repositioning during the transition, and then unhiding the sheet at the end
  of it.
- Along with the overlay sheet issue, a bug in the certificate viewer sheet is
  fixed. The code to set the overlay window's frame in |unhide| was 
  outdated. The overlay window frame gets adjusted somewhere else so 
  the code would just move the frame to an incorrect position.
BUG=558313, 556355
==========

to

==========
Fixed a few fullscreen issues:
- Removed tab glimpse at the beginning of the animation by ensuring that
the content does not autoresize during the animation.
- Fixed an overlay sheet issue during the animation. If a sheet is opened
  during the animation, it will spuriously reposition and eventually move
  behind the window. This is fixed by hiding the sheet, disabling the
  repositioning during the transition, and then unhiding the sheet at the end
  of it.
- Along with the overlay sheet issue, a bug in the certificate viewer sheet is
  fixed. The code to set the overlay window's frame in |unhide| was
  outdated. The overlay window frame gets adjusted somewhere else so
  the code would just move the frame to an incorrect position.
BUG=558313, 556355

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

[commit-bot: I haz the power, 2015-12-10 00:18:25]

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