Issue 9230011: Better fix for Lion dictionary popover cmd-W bug.

Issue 9230011: Better fix for Lion dictionary popover cmd-W bug. (Closed)

Created:
8 years, 11 months ago by Alexei Svitkine (slow)

Modified:
8 years, 11 months ago

Reviewers:
Robert Sesek

CC:
chromium-reviews

Base URL:
svn://svn.chromium.org/chrome/trunk/src/

Visibility:
Public.

More Reviews

Description

Better fix for Lion dictionary popover cmd-W bug. This reverts http://crrev.com/117681 and http://crrev.com/104931 and instead uses new 10.7 notifications to change the shortcuts on Close Tab and Close Window items, so that cmd-W does a "Close Window" when the dictionary is open. The above is also how Safari appears to solve this issue, as can be seen by going to the File menu when the dictionary popover is up. BUG=104931, 110306, 109061 TEST=1. Open a tab and double 3-finger tap on a word to bring up the dictionary popup. Hit cmd-W. The popup should close but the tab should stay open. Hit cmd-W again. The tab should close. 2. Try cmd-shift-W with > 1 tab open. The window should close. 3. Open a window that doesn't have tabs (e.g. Chrome -> About Chrome). File -> Close Tab should be disabled. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=118131

Patch Set 1 #

Total comments: 2

Patch Set 2 : '' #

Created: 8 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+60 lines, -199 lines)			Patch
M	chrome/app/nibs/MainMenu.xib	View	1	3 chunks	+9 lines, -9 lines	0 comments	Download
D	chrome/app/nibs/main_menu_unittest.mm	View	1	1 chunk	+0 lines, -60 lines	0 comments	Download
M	chrome/browser/app_controller_mac.h	View	1	1 chunk	+3 lines, -3 lines	0 comments	Download
M	chrome/browser/app_controller_mac.mm	View	1	9 chunks	+42 lines, -21 lines	0 comments	Download
M	chrome/browser/chrome_browser_main_mac.mm	View	1	1 chunk	+0 lines, -2 lines	0 comments	Download
M	chrome/browser/ui/cocoa/browser_window_controller.h	View	1	1 chunk	+0 lines, -3 lines	0 comments	Download
M	chrome/browser/ui/cocoa/browser_window_controller.mm	View	1	2 chunks	+3 lines, -4 lines	0 comments	Download
M	chrome/browser/ui/cocoa/chrome_browser_window.h	View	1	1 chunk	+0 lines, -5 lines	0 comments	Download
M	chrome/browser/ui/cocoa/chrome_browser_window.mm	View	1	2 chunks	+0 lines, -23 lines	0 comments	Download
M	chrome/browser/ui/cocoa/chrome_browser_window_unittest.mm	View	1	2 chunks	+0 lines, -57 lines	0 comments	Download
M	chrome/browser/ui/cocoa/fullscreen_window.mm	View	1	2 chunks	+2 lines, -10 lines	0 comments	Download
M	chrome/chrome_tests.gypi	View	1	1 chunk	+1 line, -2 lines	0 comments	Download

Messages

Total messages: 6 (0 generated)

Expand Messages | Collapse Messages

Robert Sesek

Sorry. I realized this dropped off my radar last night. LGTM. http://codereview.chromium.org/9230011/diff/1/chrome/browser/app_controller_mac.mm File chrome/browser/app_controller_mac.mm (right): ...

8 years, 11 months ago (2012-01-18 17:30:58 UTC) #3

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asvitkine@chromium.org/9230011/8001

8 years, 11 months ago (2012-01-18 17:57:27 UTC) #4

Alexei Svitkine (slow)

http://codereview.chromium.org/9230011/diff/1/chrome/browser/app_controller_mac.mm File chrome/browser/app_controller_mac.mm (right): http://codereview.chromium.org/9230011/diff/1/chrome/browser/app_controller_mac.mm#newcode252 chrome/browser/app_controller_mac.mm:252: [notificationCenter On 2012/01/18 17:30:59, rsesek wrote: > Could wrap ...

8 years, 11 months ago (2012-01-18 17:57:38 UTC) #5

Change committed as 118131

Expand Messages | Collapse Messages