[Mac] Remove extraneous Bookmarks menu from menu bar when running apps from launcher.

[Mac] Remove extraneous Bookmarks menu from menu bar when running apps from launcher.

When Chrome is closed, opening a bookmark app from the app launcher on
Mac leads to an additional Bookmarks menu appearing on the left of the
Mac menu bar. The issue only manifests itself under these exact
circumstances.

The problem appears to be caused by a bug in AppKit. Calling setSubmenu
on a *hidden* NSMenuItem with a *different* NSMenu to the current
submenu leads to the NSMenuItem becoming non-hidden. This precise
situation occurs when caching the bookmark menu on Mac, which is done to
improve profile switching performance.

This CL removes the extra Bookmarks menu by saving the hidden state of
the menu prior to caching, setting the menu to be visible, allowing the
existing caching to run, and then resetting the hidden state once
caching is complete. It is insufficient to skip setting the menu to be
visible in this process.

BUG=497813
Committed: https://crrev.com/b80838fdc92209c443c43a742284ada39dd2d89e
Cr-Commit-Position: refs/heads/master@{#347819}

[dominickn, 2015-09-04 04:18:39]

dominickn@chromium.org changed reviewers:
+ jackhou@chromium.org

[dominickn, 2015-09-04 04:20:22]

PTAL, thanks.

[jackhou1, 2015-09-04 07:39:48]

lgtm

It's common to prefix the CL title with "Mac:" or "[Mac]".

I'd mention in the CL description that it's an AppKit bug.

https://codereview.chromium.org/1308983009/diff/1/chrome/browser/app_controll...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/1308983009/diff/1/chrome/browser/app_controll...
chrome/browser/app_controller_mac.mm:1605: // Reset the hidden state to the
previously saved value.
Move this comment between -setSubmenu: and -setHidden:.

[dominickn, 2015-09-04 07:54:02]

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

[dominickn, 2015-09-04 07:54:02]

@rsesek: PTAL, thanks!

https://codereview.chromium.org/1308983009/diff/1/chrome/browser/app_controll...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/1308983009/diff/1/chrome/browser/app_controll...
chrome/browser/app_controller_mac.mm:1605: // Reset the hidden state to the
previously saved value.
On 2015/09/04 07:39:48, jackhou1 wrote:
> Move this comment between -setSubmenu: and -setHidden:.

Done.

[Robert Sesek, 2015-09-04 17:33:06]

https://codereview.chromium.org/1308983009/diff/20001/chrome/browser/app_cont...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/1308983009/diff/20001/chrome/browser/app_cont...
chrome/browser/app_controller_mac.mm:1589: // See http://crbug/497813 for more
details.
nit: https://crbug.com is the proper hostname to use. Just "crbug" does not
resolve.

https://codereview.chromium.org/1308983009/diff/20001/chrome/browser/app_cont...
chrome/browser/app_controller_mac.mm:1590: NSMenuItem* item = [[NSApp mainMenu]
itemWithTag:IDC_BOOKMARKS_MENU];
naming: item is a bit vague. bookmarkItem is better.

https://codereview.chromium.org/1308983009/diff/20001/chrome/browser/app_cont...
chrome/browser/app_controller_mac.mm:1606: // Reset the hidden state to the
previously saved value.
Remove this comment, since it's obvious from the code.

[dominickn, 2015-09-05 00:34:02]

Thanks!

https://codereview.chromium.org/1308983009/diff/20001/chrome/browser/app_cont...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/1308983009/diff/20001/chrome/browser/app_cont...
chrome/browser/app_controller_mac.mm:1589: // See http://crbug/497813 for more
details.
On 2015/09/04 17:33:06, Robert Sesek wrote:
> nit: https://crbug.com is the proper hostname to use. Just "crbug" does not
> resolve.

Done.

https://codereview.chromium.org/1308983009/diff/20001/chrome/browser/app_cont...
chrome/browser/app_controller_mac.mm:1590: NSMenuItem* item = [[NSApp mainMenu]
itemWithTag:IDC_BOOKMARKS_MENU];
On 2015/09/04 17:33:06, Robert Sesek wrote:
> naming: item is a bit vague. bookmarkItem is better.

Done.

https://codereview.chromium.org/1308983009/diff/20001/chrome/browser/app_cont...
chrome/browser/app_controller_mac.mm:1606: // Reset the hidden state to the
previously saved value.
On 2015/09/04 17:33:06, Robert Sesek wrote:
> Remove this comment, since it's obvious from the code.

Done.

[Robert Sesek, 2015-09-08 13:33:32]

LGTM

[dominickn, 2015-09-08 22:28:20]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2015-09-08 22:28:20]

The patchset sent to the CQ was uploaded after l-g-t-m from jackhou@chromium.org
Link to the patchset: https://codereview.chromium.org/1308983009/#ps40001
(title: "Addressing reviewer feedback")

[commit-bot: I haz the power, 2015-09-08 22:28:56]

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

[commit-bot: I haz the power, 2015-09-08 23:00:19]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-08 23:01:10]

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