Implement a Menu Item to Hide the Toolbar in Fullscreen

Replace Presentation Mode with an option to hide the toolbar in
fullscreen in 10.7+. This option can be toggled on and off under the
View menu. This option is currently disabled and hidden until a
preference for it is implemented.

BUG=544307
Committed: https://crrev.com/b8ffc7d9d185857efb041354f41590dfcb05ae27
Cr-Commit-Position: refs/heads/master@{#359968}

[spqchan, 2015-11-02 17:55:10]

Description was changed from

==========
Remove log


Added a method in exclusive context to deal with this


Removed presentation mode and added menu item for hiding the toolbar

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

to

==========
Remove log


Added a method in exclusive context to deal with this


Removed presentation mode and added menu item for hiding the toolbar

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

[spqchan, 2015-11-03 17:38:41]

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

[commit-bot: I haz the power, 2015-11-03 17:41:04]

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

[commit-bot: I haz the power, 2015-11-03 17:41:04]

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

[commit-bot: I haz the power, 2015-11-03 17:41:05]

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.

[spqchan, 2015-11-03 18:04:42]

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

[commit-bot: I haz the power, 2015-11-03 18:08:32]

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

[commit-bot: I haz the power, 2015-11-03 18:08:33]

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

[commit-bot: I haz the power, 2015-11-03 18:08:34]

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.

[spqchan, 2015-11-03 18:08:55]

Description was changed from

==========
Remove log


Added a method in exclusive context to deal with this


Removed presentation mode and added menu item for hiding the toolbar

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

to

==========
Replace Presentation Mode with an option to hide the toolbar in
fullscreen in 10.7+. This option can be toggled on and off under the
View menu.


BUG=544307
==========

[spqchan, 2015-11-03 18:30:30]

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

[spqchan, 2015-11-03 18:30:31]

PTAL

https://codereview.chromium.org/1414033009/diff/160001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_command_handler.mm (right):

https://codereview.chromium.org/1414033009/diff/160001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_command_handler.mm:210: BOOL
isFullscreenToolbarHidden = NO;
Note: this is temporary. Right now by default, the toolbar will not be hidden,
and
the value will be reset when you open a new window. 

To simplify this CL, I will be adding a pref for IDC_HIDE_FULLSCREEN_TOOLBAR 
in another CL after this one.

[Robert Sesek, 2015-11-03 20:09:38]

Is this supposed to be a persistent preference?

https://codereview.chromium.org/1414033009/diff/200001/chrome/app/chrome_comm...
File chrome/app/chrome_command_ids.h (right):

https://codereview.chromium.org/1414033009/diff/200001/chrome/app/chrome_comm...
chrome/app/chrome_command_ids.h:220: #define IDC_HIDE_FULLSCREEN_TOOLBAR    
40251
Should this be IDC_TOGGLE_FULLSCREEN_TOOLBAR ?

https://codereview.chromium.org/1414033009/diff/200001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1414033009/diff/200001/chrome/app/generated_r...
chrome/app/generated_resources.grd:12795: +        <message
name="IDS_HIDE_FULLSCREEN_TOOLBAR_MAC" desc="The Mac menu item to show/hide the
toolbar in full screen.">
Mention that this is a checkbox item, so that it's clear there is not a
corresponding Show menu item.

https://codereview.chromium.org/1414033009/diff/200001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_command_handler.mm (right):

https://codereview.chromium.org/1414033009/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_command_handler.mm:212: BOOL
isFullscreenToolbarHidden = NO;
Why is this always NO ?

https://codereview.chromium.org/1414033009/diff/200001/chrome/browser/ui/excl...
File chrome/browser/ui/exclusive_access/exclusive_access_context.h (right):

https://codereview.chromium.org/1414033009/diff/200001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/exclusive_access_context.h:36: virtual void
ToggleFullscreenToolbar();
Needs a comment.

[spqchan, 2015-11-04 01:43:42]

I just realized that the comment I added was lost a few patches ago.

To simplify this CL, I decided to just implement the function and then 
afterwards, add a pref for this in another CL. Right now, if you open a 
new window, the toolbar will be set to show by default.

However if you like, I can just add the changes for a persistent preference in
this CL.

https://codereview.chromium.org/1414033009/diff/200001/chrome/app/chrome_comm...
File chrome/app/chrome_command_ids.h (right):

https://codereview.chromium.org/1414033009/diff/200001/chrome/app/chrome_comm...
chrome/app/chrome_command_ids.h:220: #define IDC_HIDE_FULLSCREEN_TOOLBAR    
40251
On 2015/11/03 20:09:38, Robert Sesek wrote:
> Should this be IDC_TOGGLE_FULLSCREEN_TOOLBAR ?

Done.

https://codereview.chromium.org/1414033009/diff/200001/chrome/app/chrome_comm...
chrome/app/chrome_command_ids.h:220: #define IDC_HIDE_FULLSCREEN_TOOLBAR    
40251
On 2015/11/03 20:09:38, Robert Sesek wrote:
> Should this be IDC_TOGGLE_FULLSCREEN_TOOLBAR ?

Done.

https://codereview.chromium.org/1414033009/diff/200001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1414033009/diff/200001/chrome/app/generated_r...
chrome/app/generated_resources.grd:12795: +        <message
name="IDS_HIDE_FULLSCREEN_TOOLBAR_MAC" desc="The Mac menu item to show/hide the
toolbar in full screen.">
On 2015/11/03 20:09:38, Robert Sesek wrote:
> Mention that this is a checkbox item, so that it's clear there is not a
> corresponding Show menu item.

Done.

https://codereview.chromium.org/1414033009/diff/200001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_command_handler.mm (right):

https://codereview.chromium.org/1414033009/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_command_handler.mm:212: BOOL
isFullscreenToolbarHidden = NO;
On 2015/11/03 20:09:38, Robert Sesek wrote:
> Why is this always NO ?

See response

https://codereview.chromium.org/1414033009/diff/200001/chrome/browser/ui/excl...
File chrome/browser/ui/exclusive_access/exclusive_access_context.h (right):

https://codereview.chromium.org/1414033009/diff/200001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/exclusive_access_context.h:36: virtual void
ToggleFullscreenToolbar();
On 2015/11/03 20:09:38, Robert Sesek wrote:
> Needs a comment.

Done.

[Robert Sesek, 2015-11-04 21:57:16]

I think it's fine to not have the pref yet, but maybe we should do something to
the menu item in the interim (see inline comment)?

https://codereview.chromium.org/1414033009/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_command_handler.mm (right):

https://codereview.chromium.org/1414033009/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_command_handler.mm:209: case
IDC_TOGGLE_FULLSCREEN_TOOLBAR: {
Maybe make this item unconditionally disabled or hidden until it actually does
something?

https://codereview.chromium.org/1414033009/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_command_handler.mm:212: BOOL
isFullscreenToolbarHidden = NO;
Should there be a TODO for this?

[spqchan, 2015-11-04 23:59:22]

Good call, I'll disable it until I implement the preference.

https://codereview.chromium.org/1414033009/diff/220001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_command_handler.mm (right):

https://codereview.chromium.org/1414033009/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_command_handler.mm:209: case
IDC_TOGGLE_FULLSCREEN_TOOLBAR: {
On 2015/11/04 21:57:16, Robert Sesek wrote:
> Maybe make this item unconditionally disabled or hidden until it actually does
> something?

Done.

https://codereview.chromium.org/1414033009/diff/220001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_command_handler.mm:212: BOOL
isFullscreenToolbarHidden = NO;
On 2015/11/04 21:57:16, Robert Sesek wrote:
> Should there be a TODO for this?

Done.

[spqchan, 2015-11-05 00:00:09]

PTAL

[Robert Sesek, 2015-11-05 15:56:13]

LGTM

https://codereview.chromium.org/1414033009/diff/280001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_command_handler.mm (right):

https://codereview.chromium.org/1414033009/diff/280001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_command_handler.mm:31: NSInteger oldState
= [item state];
naming: should be old_state since this is a C++ function, same with new_state

[spqchan, 2015-11-05 17:50:47]

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

[spqchan, 2015-11-05 17:50:47]

PTAL

https://codereview.chromium.org/1414033009/diff/280001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_command_handler.mm (right):

https://codereview.chromium.org/1414033009/diff/280001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_command_handler.mm:31: NSInteger oldState
= [item state];
On 2015/11/05 15:56:13, Robert Sesek wrote:
> naming: should be old_state since this is a C++ function, same with new_state

Done.

[spqchan, 2015-11-05 17:51:33]

Description was changed from

==========
Replace Presentation Mode with an option to hide the toolbar in
fullscreen in 10.7+. This option can be toggled on and off under the
View menu.


BUG=544307
==========

to

==========
Replace Presentation Mode with an option to hide the toolbar in
fullscreen in 10.7+. This option can be toggled on and off under the
View menu. This option is currently disabled and hidden until a
preference for it is implemented.


BUG=544307
==========

[Robert Sesek, 2015-11-05 18:04:04]

slgtm

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

+ thakis

PTAL

[Nico, 2015-11-10 19:51:00]

you have an lg from rsesek – what do you want me to look at? everything?

[spqchan1, 2015-11-10 19:53:04]

On 2015/11/10 19:51:00, Nico wrote:
> you have an lg from rsesek – what do you want me to look at? everything?

Ah sorry for the confusion, you're the owner for some of the files here.

[spqchan1, 2015-11-10 20:12:53]

The CQ bit was checked by spqchan@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-10 20:13:28]

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

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

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

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

Dry run: Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[spqchan, 2015-11-12 17:42:53]

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

[commit-bot: I haz the power, 2015-11-12 17:43:11]

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

[commit-bot: I haz the power, 2015-11-12 17:53:57]

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

[commit-bot: I haz the power, 2015-11-12 17:53:58]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[spqchan, 2015-11-12 18:10:24]

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

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

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

[spqchan, 2015-11-12 19:12:14]

On 2015/11/12 18:11:24, commit-bot: I haz the power wrote:
> Dry run: CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1414033009/360001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1414033009/360001

Just realized that a test is broken. Fixing the test right now

[commit-bot: I haz the power, 2015-11-12 19:44:30]

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

[commit-bot: I haz the power, 2015-11-12 19:44:31]

Dry run: 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-11-13 01:34:48]

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

[commit-bot: I haz the power, 2015-11-13 01:38:37]

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

[commit-bot: I haz the power, 2015-11-13 01:47:33]

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

[commit-bot: I haz the power, 2015-11-13 01:47:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[spqchan, 2015-11-13 02:17:40]

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

[commit-bot: I haz the power, 2015-11-13 02:18:30]

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

[commit-bot: I haz the power, 2015-11-13 02:29:31]

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

[commit-bot: I haz the power, 2015-11-13 02:29:32]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[spqchan, 2015-11-13 18:16:46]

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

[commit-bot: I haz the power, 2015-11-13 18:17:26]

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

[commit-bot: I haz the power, 2015-11-13 19:13:20]

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

[commit-bot: I haz the power, 2015-11-13 19:13:21]

Dry run: This issue passed the CQ dry run.

[spqchan, 2015-11-13 20:45:43]

On 2015/11/13 19:13:21, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

Fixed the merge conflicts and the tests.

[Nico, 2015-11-16 22:10:46]

lgtm

I looked at everything except 	chrome/browser/ui/cocoa/*

Is there a browser test for toggling this? (doesn't have to be in this cl)

[spqchan, 2015-11-16 22:15:43]

On 2015/11/16 22:10:46, Nico wrote:
> lgtm
> 
> I looked at everything except 	chrome/browser/ui/cocoa/*
> 
> Is there a browser test for toggling this? (doesn't have to be in this cl)

Thanks! At this moment no. I'm planning to put that in another CL.

[spqchan, 2015-11-16 22:15:51]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2015-11-16 22:15:51]

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/1414033009/#ps400001
(title: "Rebase and merge code")

[commit-bot: I haz the power, 2015-11-16 22:16:33]

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

[commit-bot: I haz the power, 2015-11-16 22:27:32]

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

[commit-bot: I haz the power, 2015-11-16 22:27:33]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[spqchan1, 2015-11-16 22:47:09]

The CQ bit was checked by spqchan@google.com

[commit-bot: I haz the power, 2015-11-16 22:47:23]

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

[commit-bot: I haz the power, 2015-11-16 22:54:34]

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

[commit-bot: I haz the power, 2015-11-16 22:54:35]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[spqchan, 2015-11-17 00:25:19]

The CQ bit was checked by spqchan@chromium.org

[commit-bot: I haz the power, 2015-11-17 00:26:47]

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

[commit-bot: I haz the power, 2015-11-17 01:17:29]

Committed patchset #21 (id:400001)

[commit-bot: I haz the power, 2015-11-17 01:18:22]

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