Modified the behavior of "Hide Toolbar in Full Screen"

Modified the behavior of "Hide Toolbar in Full Screen"

Changed the string to "Always Show Toolbar in Full Screen" and
reversed the logic. Modified the behavior so that instead of changing
just the current window, toggling the menu item will update all of the
windows associated with active profile.

BUG=588110
Committed: https://crrev.com/a04af1f6fbb8be7cb87efddd6f715ae51ea9138f
Cr-Commit-Position: refs/heads/master@{#378770}

[spqchan, 2016-02-25 22:38:16]

Description was changed from

==========
Modified the behavior of "Hide Toolbar in Full Screen"

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

to

==========
Modified the behavior of "Hide Toolbar in Full Screen"

Changed the string to "Always Show Toolbar in Full Screen" and the
behavior so that toggling the menu item will update all of the windows
associated with active profile. 

BUG=588110
==========

[spqchan, 2016-02-25 22:46:52]

Description was changed from

==========
Modified the behavior of "Hide Toolbar in Full Screen"

Changed the string to "Always Show Toolbar in Full Screen" and the
behavior so that toggling the menu item will update all of the windows
associated with active profile. 

BUG=588110
==========

to

==========
Modified the behavior of "Hide Toolbar in Full Screen"

Changed the string to "Always Show Toolbar in Full Screen" and reversed the
logic. Modified the behavior that instead of changing just
the current window, toggling the menu item will update all of the windows
associated with active profile. 

BUG=588110
==========

[spqchan, 2016-02-25 23:08:41]

Description was changed from

==========
Modified the behavior of "Hide Toolbar in Full Screen"

Changed the string to "Always Show Toolbar in Full Screen" and reversed the
logic. Modified the behavior that instead of changing just
the current window, toggling the menu item will update all of the windows
associated with active profile. 

BUG=588110
==========

to

==========
Modified the behavior of "Hide Toolbar in Full Screen"

Changed the string to "Always Show Toolbar in Full Screen" and 
reversed the logic. Modified the behavior that instead of changing just
the current window, toggling the menu item will update all of the 
windows associated with active profile. 

BUG=588110
==========

[spqchan, 2016-02-25 23:11:24]

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

[spqchan, 2016-02-25 23:11:25]

PTAL

[Robert Sesek, 2016-02-26 20:42:28]

https://codereview.chromium.org/1737413002/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1737413002/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:438: profile_pref_registrar_.Add(
Seems like this should be limited to OS_MACOSX? But does this really belong
here? Could the EAC register for this itself?

https://codereview.chromium.org/1737413002/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_commands_mac.cc (right):

https://codereview.chromium.org/1737413002/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_commands_mac.cc:33: bool showToolbar =
!prefs->GetBoolean(prefs::kShowFullscreenToolbar);
naming: show_toolbar

https://codereview.chromium.org/1737413002/diff/40001/chrome/common/pref_name...
File chrome/common/pref_names.cc (left):

https://codereview.chromium.org/1737413002/diff/40001/chrome/common/pref_name...
chrome/common/pref_names.cc:390: const char kHideFullscreenToolbar[] =
"browser.hide_fullscreen_toolbar";
Did this pref make it to stable? Should the value be migrated?

[spqchan, 2016-02-26 23:24:59]

PTAL

https://codereview.chromium.org/1737413002/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1737413002/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:438: profile_pref_registrar_.Add(
On 2016/02/26 20:42:27, Robert Sesek wrote:
> Seems like this should be limited to OS_MACOSX? But does this really belong
> here? Could the EAC register for this itself?

Good point. I was following what "Always Show Bookmarks" was doing so I think
it's fine to put it here.
It makes more sense though to put it in EAC considering it's OS_MACOSX specific.

https://codereview.chromium.org/1737413002/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_commands_mac.cc (right):

https://codereview.chromium.org/1737413002/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_commands_mac.cc:33: bool showToolbar =
!prefs->GetBoolean(prefs::kShowFullscreenToolbar);
On 2016/02/26 20:42:27, Robert Sesek wrote:
> naming: show_toolbar

Done.

https://codereview.chromium.org/1737413002/diff/40001/chrome/common/pref_name...
File chrome/common/pref_names.cc (left):

https://codereview.chromium.org/1737413002/diff/40001/chrome/common/pref_name...
chrome/common/pref_names.cc:390: const char kHideFullscreenToolbar[] =
"browser.hide_fullscreen_toolbar";
On 2016/02/26 20:42:27, Robert Sesek wrote:
> Did this pref make it to stable? Should the value be migrated?

Not yet, but given the amount of time we have left, it may as well be.
That's a good point, how do we migrate these values?
It might be safe to not migrate this value since it's pretty minor.

Alternatively, I can change it back to the hide_fullscreen_toolbar. I'm
concerned that it will be confusing in the codebase though.

[Robert Sesek, 2016-02-29 18:05:35]

https://codereview.chromium.org/1737413002/diff/40001/chrome/common/pref_name...
File chrome/common/pref_names.cc (left):

https://codereview.chromium.org/1737413002/diff/40001/chrome/common/pref_name...
chrome/common/pref_names.cc:390: const char kHideFullscreenToolbar[] =
"browser.hide_fullscreen_toolbar";
On 2016/02/26 23:24:59, spqchan wrote:
> On 2016/02/26 20:42:27, Robert Sesek wrote:
> > Did this pref make it to stable? Should the value be migrated?
> 
> Not yet, but given the amount of time we have left, it may as well be.
> That's a good point, how do we migrate these values?
> It might be safe to not migrate this value since it's pretty minor.

Will it make it to stable? If not, then I don't think the migration is worth it.
If this pref will make it to stable, we should migrate it IMO.

The way to do that is to keep register the new pref, migrate the value from the
old pref, clear the old pref (to prevent migration more than once), and then in
a follow-up milestone, delete the old pref.

> Alternatively, I can change it back to the hide_fullscreen_toolbar. I'm
> concerned that it will be confusing in the codebase though.

Agreed.

https://codereview.chromium.org/1737413002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm
(right):

https://codereview.chromium.org/1737413002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm:34:
pref_registrar_.RemoveAll();
I think this is done in the dtor automatically.

[spqchan, 2016-02-29 21:20:09]

Alright, I migrated the pref values in browser_commands_mac.cc. Would that be an
appropriate place to do it?
I'm checking to see if the old pref is not set to the default value.
If it's not, then I migrate the value to the new pref and then clear the old
one.

I considered migrating it in browser_ui_prefs.cc but I couldn't find a way to
grab the existing value from PrefRegistrySimple.
I could try to get the current PrefService, and then grab the value from it, but
it doesn't seem right.

https://codereview.chromium.org/1737413002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm
(right):

https://codereview.chromium.org/1737413002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm:34:
pref_registrar_.RemoveAll();
On 2016/02/29 18:05:34, Robert Sesek wrote:
> I think this is done in the dtor automatically.

Done.

[Robert Sesek, 2016-02-29 22:52:10]

LGTM

[spqchan, 2016-02-29 23:17:33]

The CQ bit was checked by spqchan@chromium.org

[commit-bot: I haz the power, 2016-02-29 23:17:49]

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

[spqchan, 2016-02-29 23:18:29]

The CQ bit was unchecked by spqchan@chromium.org

[spqchan, 2016-02-29 23:20:09]

Description was changed from

==========
Modified the behavior of "Hide Toolbar in Full Screen"

Changed the string to "Always Show Toolbar in Full Screen" and 
reversed the logic. Modified the behavior that instead of changing just
the current window, toggling the menu item will update all of the 
windows associated with active profile. 

BUG=588110
==========

to

==========
Modified the behavior of "Hide Toolbar in Full Screen"

Changed the string to "Always Show Toolbar in Full Screen" and 
reversed the logic. Modified the behavior so that instead of changing
just the current window, toggling the menu item will update all of the 
windows associated with active profile. 

BUG=588110
==========

[spqchan, 2016-02-29 23:26:30]

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

[spqchan, 2016-02-29 23:26:31]

+msw for OWNER of the following files:

chrome/browser/ui/browser_commands_mac.cc
chrome/browser/ui/browser_ui_prefs.cc
chrome/browser/ui/exclusive_access/exclusive_access_context.cc
chrome/browser/ui/exclusive_access/exclusive_access_context.h

[msw, 2016-03-01 00:07:15]

https://codereview.chromium.org/1737413002/diff/100001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_commands_mac.cc (right):

https://codereview.chromium.org/1737413002/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser_commands_mac.cc:33: bool show_toolbar =
!prefs->GetBoolean(prefs::kShowFullscreenToolbar);
This '!' inversion is confusing... did you mean to get kHideFullscreenToolbar
instead? or is the inversion intended? can you add a comment or explain?

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

https://codereview.chromium.org/1737413002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm:84: bool
hideToolbar = !prefs->GetBoolean(prefs::kShowFullscreenToolbar);
This inversion seems correct, but the old logic also had an inversion for the
old pref; I'm confused, can you clarify?

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

https://codereview.chromium.org/1737413002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:700: // Tests
that the preference is set to false by default.
nit: update comment

[msw, 2016-03-01 00:08:51]

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

https://codereview.chromium.org/1737413002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_command_handler.mm:59: // Migrate the
value of kHideFullscreenToolbar to kShowFullscreenToolbar if
Do we only migrate when the user toggles the value? that doesn't seem right...

[spqchan, 2016-03-01 17:50:18]

PTAL

https://codereview.chromium.org/1737413002/diff/100001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_commands_mac.cc (right):

https://codereview.chromium.org/1737413002/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser_commands_mac.cc:33: bool show_toolbar =
!prefs->GetBoolean(prefs::kShowFullscreenToolbar);
On 2016/03/01 00:07:15, msw wrote:
> This '!' inversion is confusing... did you mean to get kHideFullscreenToolbar
> instead? or is the inversion intended? can you add a comment or explain?

Done.

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

https://codereview.chromium.org/1737413002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm:84: bool
hideToolbar = !prefs->GetBoolean(prefs::kShowFullscreenToolbar);
On 2016/03/01 00:07:15, msw wrote:
> This inversion seems correct, but the old logic also had an inversion for the
> old pref; I'm confused, can you clarify?

Sorry for the confusion, the old logic was different because it was in charge of
toggling the preference.
As such, it would get the value, invert it, set the toolbar in the window
according to it and then change the preference to the new value.

The new logic will only retrieves the variable and update the toolbar.
Since it no longer need to toggle the variable, there's no need to invert it.

Anyway, I just tried to make things a bit more clear by changing "hideToolbar"
to "showToolbar" in the BrowserWindowController.

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

https://codereview.chromium.org/1737413002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_command_handler.mm:59: // Migrate the
value of kHideFullscreenToolbar to kShowFullscreenToolbar if
On 2016/03/01 00:08:51, msw wrote:
> Do we only migrate when the user toggles the value? that doesn't seem right...

No, this is called when the menu is created. The method is called to make sure
that the checkmark state of the menu items are set correctly. You're right
though, it makes more sense for this to be migrated when the browser is created,
not when the menu is created. Moved it to browser.cc

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

https://codereview.chromium.org/1737413002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:700: // Tests
that the preference is set to false by default.
On 2016/03/01 00:07:15, msw wrote:
> nit: update comment

Done.

[msw, 2016-03-01 19:23:03]

One big comment; some nits.

https://codereview.chromium.org/1737413002/diff/120001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1737413002/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:441: // Migrate the value of kHideFullscreenToolbar
to kShowFullscreenToolbar if
I think this belongs in MigrateObsoleteProfilePrefs.

https://codereview.chromium.org/1737413002/diff/120001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_commands_mac.cc (right):

https://codereview.chromium.org/1737413002/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser_commands_mac.cc:7: #include "base/command_line.h"
nit: remove

https://codereview.chromium.org/1737413002/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser_commands_mac.cc:12: #include
"chrome/browser/ui/exclusive_access/exclusive_access_context.h"
nit: remove

https://codereview.chromium.org/1737413002/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser_commands_mac.cc:14: #include
"chrome/common/chrome_switches.h"
nit: remove

https://codereview.chromium.org/1737413002/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser_commands_mac.cc:22: if
(chrome::mac::SupportsSystemFullscreen())
unrelated nit: not sure 'chrome::' is needed here.

https://codereview.chromium.org/1737413002/diff/120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

https://codereview.chromium.org/1737413002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller.h:537: // Set toolbar's
visibility in fullscreen mode.
nit: "Set the toolbar's"

https://codereview.chromium.org/1737413002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller.h:564: // Whether if the
toolbar should be shown in fullscreen.
nit: remove 'if'

https://codereview.chromium.org/1737413002/diff/120001/chrome/common/pref_nam...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/1737413002/diff/120001/chrome/common/pref_nam...
chrome/common/pref_names.cc:392: // TODO(spqchan): Remove this in M51. See
crbug.com/590827.
nit: M51 seems too soon now; M50 already branched.

[spqchan, 2016-03-01 23:41:31]

PTAL

https://codereview.chromium.org/1737413002/diff/120001/chrome/browser/ui/brow...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1737413002/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:441: // Migrate the value of kHideFullscreenToolbar
to kShowFullscreenToolbar if
On 2016/03/01 19:23:02, msw wrote:
> I think this belongs in MigrateObsoleteProfilePrefs.

Sounds good! I moved the code into MigrateObsoleteProfilePrefs.

https://codereview.chromium.org/1737413002/diff/120001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_commands_mac.cc (right):

https://codereview.chromium.org/1737413002/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser_commands_mac.cc:7: #include "base/command_line.h"
On 2016/03/01 19:23:02, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/1737413002/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser_commands_mac.cc:12: #include
"chrome/browser/ui/exclusive_access/exclusive_access_context.h"
On 2016/03/01 19:23:02, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/1737413002/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser_commands_mac.cc:14: #include
"chrome/common/chrome_switches.h"
On 2016/03/01 19:23:02, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/1737413002/diff/120001/chrome/browser/ui/brow...
chrome/browser/ui/browser_commands_mac.cc:22: if
(chrome::mac::SupportsSystemFullscreen())
On 2016/03/01 19:23:02, msw wrote:
> unrelated nit: not sure 'chrome::' is needed here.

Done.

https://codereview.chromium.org/1737413002/diff/120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

https://codereview.chromium.org/1737413002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller.h:537: // Set toolbar's
visibility in fullscreen mode.
On 2016/03/01 19:23:02, msw wrote:
> nit: "Set the toolbar's"

Done.

https://codereview.chromium.org/1737413002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller.h:564: // Whether if the
toolbar should be shown in fullscreen.
On 2016/03/01 19:23:02, msw wrote:
> nit: remove 'if'

Done.

https://codereview.chromium.org/1737413002/diff/120001/chrome/common/pref_nam...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/1737413002/diff/120001/chrome/common/pref_nam...
chrome/common/pref_names.cc:392: // TODO(spqchan): Remove this in M51. See
crbug.com/590827.
On 2016/03/01 19:23:02, msw wrote:
> nit: M51 seems too soon now; M50 already branched.

Done.

[msw, 2016-03-01 23:44:20]

lgtm with a minor nit

https://codereview.chromium.org/1737413002/diff/140001/chrome/browser/prefs/b...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/1737413002/diff/140001/chrome/browser/prefs/b...
chrome/browser/prefs/browser_prefs.cc:631: // Added 2/2016.
nit: 03/2016; it's march already :)

[spqchan, 2016-03-01 23:52:09]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2016-03-01 23:52:09]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1737413002/#ps160001
(title: "Feb -> March")

[commit-bot: I haz the power, 2016-03-01 23:52:51]

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

[commit-bot: I haz the power, 2016-03-02 00:10:40]

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

[commit-bot: I haz the power, 2016-03-02 00:10:41]

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

[spqchan, 2016-03-02 01:09:32]

+jochen for OWNER on browser_prefs.cc

[spqchan, 2016-03-02 01:09:56]

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

[spqchan, 2016-03-02 01:09:57]

+jochen for OWNER on browser_prefs.cc

[jochen (gone - plz use gerrit), 2016-03-02 11:01:12]

lgtm

[spqchan, 2016-03-02 17:37:32]

The CQ bit was checked by spqchan@chromium.org

[commit-bot: I haz the power, 2016-03-02 17:38:03]

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

[commit-bot: I haz the power, 2016-03-02 17:43:16]

Description was changed from

==========
Modified the behavior of "Hide Toolbar in Full Screen"

Changed the string to "Always Show Toolbar in Full Screen" and 
reversed the logic. Modified the behavior so that instead of changing
just the current window, toggling the menu item will update all of the 
windows associated with active profile. 

BUG=588110
==========

to

==========
Modified the behavior of "Hide Toolbar in Full Screen"

Changed the string to "Always Show Toolbar in Full Screen" and 
reversed the logic. Modified the behavior so that instead of changing
just the current window, toggling the menu item will update all of the 
windows associated with active profile. 

BUG=588110
==========

[commit-bot: I haz the power, 2016-03-02 17:43:17]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-03-02 17:44:35]

Description was changed from

==========
Modified the behavior of "Hide Toolbar in Full Screen"

Changed the string to "Always Show Toolbar in Full Screen" and 
reversed the logic. Modified the behavior so that instead of changing
just the current window, toggling the menu item will update all of the 
windows associated with active profile. 

BUG=588110
==========

to

==========
Modified the behavior of "Hide Toolbar in Full Screen"

Changed the string to "Always Show Toolbar in Full Screen" and
reversed the logic. Modified the behavior so that instead of changing
just the current window, toggling the menu item will update all of the
windows associated with active profile.

BUG=588110

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

[commit-bot: I haz the power, 2016-03-02 17:44:39]

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