[Mac] Reveal Fullscreen Toolbar for Tab Strip Changes

[Mac] Reveal Fullscreen Toolbar for Tab Strip Changes

Reveal the fullscreen toolbar for a short duration when the tabstrip has
changed. This feature is hidden behind the
enable-fullscreen-toolbar-reveal flag.

BUG=591776
Tests= Run
BrowserWindowControllerTest.FullscreenToolbarExposedForTabstripChanges

Committed: https://crrev.com/7f53ca50f95788ed49dae6134dd92a69c2975b1e
Cr-Commit-Position: refs/heads/master@{#402956}

[spqchan, 2016-06-23 02:33:28]

Description was changed from

==========
Reveal Toolbar for Tabstrip Changes
BUG=
==========

to

==========
[Mac] Reveal Fullscreen Toolbar for Tab Strip Changes
BUG=
==========

[spqchan, 2016-06-23 02:34:44]

Description was changed from

==========
[Mac] Reveal Fullscreen Toolbar for Tab Strip Changes
BUG=
==========

to

==========
[Mac] Reveal Fullscreen Toolbar for Tab Strip Changes

Reveal the fullscreen toolbar for a short duration when the tabstrip has
changed. This feature is hidden behind the

BUG=591776
==========

[spqchan, 2016-06-23 02:35:20]

Description was changed from

==========
[Mac] Reveal Fullscreen Toolbar for Tab Strip Changes

Reveal the fullscreen toolbar for a short duration when the tabstrip has
changed. This feature is hidden behind the

BUG=591776
==========

to

==========
[Mac] Reveal Fullscreen Toolbar for Tab Strip Changes

Reveal the fullscreen toolbar for a short duration when the tabstrip has
changed. This feature is hidden behind the
enable-fullscreen-toolbar-reveal flag.

BUG=591776
==========

[spqchan, 2016-06-23 02:35:20]

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

[spqchan, 2016-06-23 02:36:04]

PTAL

[Robert Sesek, 2016-06-23 14:08:24]

Is it possible to write a unittest/browsertest for this?

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

https://codereview.chromium.org/2086273003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:1035: bool
will_focus_location_bar =
naming: camelCase

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

https://codereview.chromium.org/2086273003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/presentation_mode_controller.mm:397: [self
ensureOverlayShownWithAnimation:YES delay:YES];
What if multiple tabs are inserted and this method calls more than once before
the hideTimer_ fires?

https://codereview.chromium.org/2086273003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/presentation_mode_controller.mm:509:
revealToolbarForTabStripChanges_ = false;
false -> NO

[spqchan, 2016-06-23 19:41:52]

PTAL

I'm not sure how to write a test for this, since it's all animated.
Are there existing tests for animations like this?

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

https://codereview.chromium.org/2086273003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:1035: bool
will_focus_location_bar =
On 2016/06/23 14:08:24, Robert Sesek wrote:
> naming: camelCase

Done.

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

https://codereview.chromium.org/2086273003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/presentation_mode_controller.mm:397: [self
ensureOverlayShownWithAnimation:YES delay:YES];
On 2016/06/23 14:08:24, Robert Sesek wrote:
> What if multiple tabs are inserted and this method calls more than once before
> the hideTimer_ fires?

If the method calls before it fires, then hideTimer gets invalidated before
showTimer gets kickstarted.
I removed the delay so that the dropdown will appear right away.

https://codereview.chromium.org/2086273003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/presentation_mode_controller.mm:509:
revealToolbarForTabStripChanges_ = false;
On 2016/06/23 14:08:24, Robert Sesek wrote:
> false -> NO

Done.

[Robert Sesek, 2016-06-24 18:09:48]

On 2016/06/23 19:41:52, spqchan wrote:
> PTAL
> 
> I'm not sure how to write a test for this, since it's all animated.
> Are there existing tests for animations like this?

We have some unittests here:
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/browser_window_c....
And some browsertests here (though it's a bit :( that most of the fullscreen
ones are DISABLED_):
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/browser_window_c...

The core of what we want to test is that when a tab is inserted that we start
the bar animation. Maybe we don't need the animation and a fake/mock would do?

[spqchan, 2016-06-28 22:35:56]

PTAL

Thanks for the suggestions! I added a browser test

[spqchan, 2016-06-28 22:47:16]

Description was changed from

==========
[Mac] Reveal Fullscreen Toolbar for Tab Strip Changes

Reveal the fullscreen toolbar for a short duration when the tabstrip has
changed. This feature is hidden behind the
enable-fullscreen-toolbar-reveal flag.

BUG=591776
==========

to

==========
[Mac] Reveal Fullscreen Toolbar for Tab Strip Changes

Reveal the fullscreen toolbar for a short duration when the tabstrip has
changed. This feature is hidden behind the
enable-fullscreen-toolbar-reveal flag.

BUG=591776
Tests=
BrowserWindowControllerTest.FullscreenToolbarExposedForTabstripChanges
==========

[spqchan, 2016-06-28 22:47:26]

Description was changed from

==========
[Mac] Reveal Fullscreen Toolbar for Tab Strip Changes

Reveal the fullscreen toolbar for a short duration when the tabstrip has
changed. This feature is hidden behind the
enable-fullscreen-toolbar-reveal flag.

BUG=591776
Tests=
BrowserWindowControllerTest.FullscreenToolbarExposedForTabstripChanges
==========

to

==========
[Mac] Reveal Fullscreen Toolbar for Tab Strip Changes

Reveal the fullscreen toolbar for a short duration when the tabstrip has
changed. This feature is hidden behind the
enable-fullscreen-toolbar-reveal flag.

BUG=591776
Tests= Run
BrowserWindowControllerTest.FullscreenToolbarExposedForTabstripChanges
==========

[Robert Sesek, 2016-06-28 23:11:26]

Nice! Thanks for adding a test. Just 1 comment:

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

https://codereview.chromium.org/2086273003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:1842:
presentationModeController_.reset(controller);
Since you're taking ownership, I'd make the argument a scoped_nsobject or retain
|controller| here.

[spqchan, 2016-06-29 00:35:50]

PTAL

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

https://codereview.chromium.org/2086273003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:1842:
presentationModeController_.reset(controller);
On 2016/06/28 23:11:26, Robert Sesek wrote:
> Since you're taking ownership, I'd make the argument a scoped_nsobject or
retain
> |controller| here.

Done.

[Robert Sesek, 2016-06-29 00:57:28]

LGTM

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

https://codereview.chromium.org/2086273003/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:787:
[[MockPresentationModeController alloc]
This needs to be in scoped_nsobject now.

[spqchan, 2016-06-29 01:17:56]

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

[spqchan, 2016-06-29 01:17:57]

+isherman for histogram.xml

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

https://codereview.chromium.org/2086273003/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:787:
[[MockPresentationModeController alloc]
On 2016/06/29 00:57:28, Robert Sesek wrote:
> This needs to be in scoped_nsobject now.

Done.

[Ilya Sherman, 2016-06-29 20:32:02]

histograms.xml lgtm

[spqchan, 2016-06-29 20:34:07]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2016-06-29 20:34:08]

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/2086273003/#ps120001
(title: "scoped_nsobject")

[spqchan, 2016-06-29 20:34:22]

thanks

[commit-bot: I haz the power, 2016-06-29 20:34:57]

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-06-29 22:27:05]

Description was changed from

==========
[Mac] Reveal Fullscreen Toolbar for Tab Strip Changes

Reveal the fullscreen toolbar for a short duration when the tabstrip has
changed. This feature is hidden behind the
enable-fullscreen-toolbar-reveal flag.

BUG=591776
Tests= Run
BrowserWindowControllerTest.FullscreenToolbarExposedForTabstripChanges
==========

to

==========
[Mac] Reveal Fullscreen Toolbar for Tab Strip Changes

Reveal the fullscreen toolbar for a short duration when the tabstrip has
changed. This feature is hidden behind the
enable-fullscreen-toolbar-reveal flag.

BUG=591776
Tests= Run
BrowserWindowControllerTest.FullscreenToolbarExposedForTabstripChanges
==========

[commit-bot: I haz the power, 2016-06-29 22:27:06]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-29 22:27:22]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-29 22:29:45]

Description was changed from

==========
[Mac] Reveal Fullscreen Toolbar for Tab Strip Changes

Reveal the fullscreen toolbar for a short duration when the tabstrip has
changed. This feature is hidden behind the
enable-fullscreen-toolbar-reveal flag.

BUG=591776
Tests= Run
BrowserWindowControllerTest.FullscreenToolbarExposedForTabstripChanges
==========

to

==========
[Mac] Reveal Fullscreen Toolbar for Tab Strip Changes

Reveal the fullscreen toolbar for a short duration when the tabstrip has
changed. This feature is hidden behind the
enable-fullscreen-toolbar-reveal flag.

BUG=591776
Tests= Run
BrowserWindowControllerTest.FullscreenToolbarExposedForTabstripChanges

Committed: https://crrev.com/7f53ca50f95788ed49dae6134dd92a69c2975b1e
Cr-Commit-Position: refs/heads/master@{#402956}
==========

[commit-bot: I haz the power, 2016-06-29 22:29:47]

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