Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(82)

Issue 1114803002: Mac: App menuBars: Ignore closing windows if they are not main (Closed)

Created:
5 years, 7 months ago by tapted
Modified:
5 years, 7 months ago
Reviewers:
Mark Mentovai
CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Mac: App menuBars: Ignore closing windows if they are not main Currently when a window that is not main (e.g. the app list) completes a fade-out animation and closes, it will trigger a mainMenu update. We need to ensure the mainMenu says "Chrome" when the last window on the active space is closed. But if that didn't have main status, it won't have been influencing the mainMenu to begin with and also won't trigger a later NSWindowDidBecomeMainNotification. This regressed in r323157, but the bug was always present. It was previously avoided because updates were skipped by anticipating whether AppKit would make a subsequent window main. But that's hard to get right and conflicts with behaviour used when closing panel windows. BUG=481803 TEST=On Mac, open a packaged app window with the app launcher. When launched for the first time, the menubar should say the apps name, rather than "Chrome". Committed: https://crrev.com/5ac229eeb3293bdb5f3cfa4c6c7c3a98cca123df Cr-Commit-Position: refs/heads/master@{#327474}

Patch Set 1 #

Patch Set 2 : add test #

Patch Set 3 : Urgh focus #

Patch Set 4 : cl format #

Unified diffs Side-by-side diffs Delta from patch set Stats (+80 lines, -13 lines) Patch
M chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm View 1 chunk +12 lines, -11 lines 0 comments Download
M chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm View 1 2 3 5 chunks +68 lines, -2 lines 0 comments Download

Messages

Total messages: 9 (4 generated)
tapted
Hi Mark, please take a look (note: planning to merge to m43). Just a one-line ...
5 years, 7 months ago (2015-04-29 05:32:23 UTC) #4
Mark Mentovai
LGTM
5 years, 7 months ago (2015-04-29 11:30:56 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1114803002/80001
5 years, 7 months ago (2015-04-29 12:11:51 UTC) #7
commit-bot: I haz the power
Committed patchset #4 (id:80001)
5 years, 7 months ago (2015-04-29 12:17:44 UTC) #8
commit-bot: I haz the power
5 years, 7 months ago (2015-04-29 12:18:55 UTC) #9
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/5ac229eeb3293bdb5f3cfa4c6c7c3a98cca123df
Cr-Commit-Position: refs/heads/master@{#327474}

Powered by Google App Engine
This is Rietveld 408576698