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

Issue 3137013: [Mac] Implement highlight-on-hover for the Wrench menu buttons. (Closed)

Created:
10 years, 4 months ago by Robert Sesek
Modified:
9 years, 7 months ago
CC:
chromium-reviews, John Grabowski, pam+watch_chromium.org, ben+cc_chromium.org
Base URL:
http://src.chromium.org/git/chromium.git
Visibility:
Public.

Description

[Mac] Implement highlight-on-hover for the Wrench menu buttons. R=pinkerton, shess, mark BUG=51643 TEST=Open Wrench menu in sticky and non-sticky mode. Hover over buttons and they highlight. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=56430

Patch Set 1 #

Total comments: 8

Patch Set 2 : Fix conversion method #

Total comments: 2

Patch Set 3 : Disable the timer if on 10.5 #

Total comments: 3

Patch Set 4 : Addressed all comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+96 lines, -1 line) Patch
M chrome/browser/cocoa/menu_tracked_button.h View 2 chunks +13 lines, -0 lines 0 comments Download
M chrome/browser/cocoa/menu_tracked_button.mm View 1 2 3 1 chunk +83 lines, -1 line 0 comments Download

Messages

Total messages: 15 (0 generated)
Robert Sesek
10 years, 4 months ago (2010-08-17 15:01:34 UTC) #1
pink (ping after 24hrs)
http://codereview.chromium.org/3137013/diff/1/3 File chrome/browser/cocoa/menu_tracked_button.mm (right): http://codereview.chromium.org/3137013/diff/1/3#newcode56 chrome/browser/cocoa/menu_tracked_button.mm:56: afterDelay:0.05 // This is the smallest delay that works. ...
10 years, 4 months ago (2010-08-17 17:41:00 UTC) #2
Robert Sesek
http://codereview.chromium.org/3137013/diff/1/3 File chrome/browser/cocoa/menu_tracked_button.mm (right): http://codereview.chromium.org/3137013/diff/1/3#newcode56 chrome/browser/cocoa/menu_tracked_button.mm:56: afterDelay:0.05 // This is the smallest delay that works. ...
10 years, 4 months ago (2010-08-17 18:18:04 UTC) #3
pink (ping after 24hrs)
http://codereview.chromium.org/3137013/diff/1/3 File chrome/browser/cocoa/menu_tracked_button.mm (right): http://codereview.chromium.org/3137013/diff/1/3#newcode81 chrome/browser/cocoa/menu_tracked_button.mm:81: point = [[self window] convertScreenToBase:point]; we've had a lot ...
10 years, 4 months ago (2010-08-17 18:27:53 UTC) #4
Scott Hess - ex-Googler
drive-by http://codereview.chromium.org/3137013/diff/1/3 File chrome/browser/cocoa/menu_tracked_button.mm (right): http://codereview.chromium.org/3137013/diff/1/3#newcode82 chrome/browser/cocoa/menu_tracked_button.mm:82: point = [self convertPointFromBase:point]; I find pink's comment ...
10 years, 4 months ago (2010-08-17 19:55:07 UTC) #5
pink (ping after 24hrs)
No, the event's mouse location is already in the window's coordinate system, hence the need ...
10 years, 4 months ago (2010-08-17 19:56:41 UTC) #6
Robert Sesek
On Tue, Aug 17, 2010 at 3:56 PM, Mike Pinkerton <pinkerton@chromium.org>wrote: > No, the event's ...
10 years, 4 months ago (2010-08-17 19:59:51 UTC) #7
pink (ping after 24hrs)
per irc, we were all correct in one way or another :-) On Tue, Aug ...
10 years, 4 months ago (2010-08-17 20:06:35 UTC) #8
Robert Sesek
Updated to keep the conversion from screen to base, but use convertPoint:fromView: to get to ...
10 years, 4 months ago (2010-08-17 20:23:39 UTC) #9
pink (ping after 24hrs)
http://codereview.chromium.org/3137013/diff/11001/5003 File chrome/browser/cocoa/menu_tracked_button.mm (right): http://codereview.chromium.org/3137013/diff/11001/5003#newcode79 chrome/browser/cocoa/menu_tracked_button.mm:79: if (floor(NSAppKitVersionNumber) <= NSAppKitVersionNumber10_5) { Is this the right ...
10 years, 4 months ago (2010-08-17 20:41:04 UTC) #10
Robert Sesek
http://codereview.chromium.org/3137013/diff/11001/5003 File chrome/browser/cocoa/menu_tracked_button.mm (right): http://codereview.chromium.org/3137013/diff/11001/5003#newcode79 chrome/browser/cocoa/menu_tracked_button.mm:79: if (floor(NSAppKitVersionNumber) <= NSAppKitVersionNumber10_5) { On 2010/08/17 20:41:04, pink ...
10 years, 4 months ago (2010-08-17 20:53:51 UTC) #11
pink (ping after 24hrs)
lgtm http://codereview.chromium.org/3137013/diff/15001/16002 File chrome/browser/cocoa/menu_tracked_button.mm (right): http://codereview.chromium.org/3137013/diff/15001/16002#newcode14 chrome/browser/cocoa/menu_tracked_button.mm:14: // Apple does not define NSAppKitVersionNumber10_5 when using ...
10 years, 4 months ago (2010-08-17 21:13:56 UTC) #12
Mark Mentovai
http://codereview.chromium.org/3137013/diff/15001/16002 File chrome/browser/cocoa/menu_tracked_button.mm (right): http://codereview.chromium.org/3137013/diff/15001/16002#newcode63 chrome/browser/cocoa/menu_tracked_button.mm:63: afterDelay:0.05 // This is the largest delay that works. ...
10 years, 4 months ago (2010-08-17 21:17:58 UTC) #13
Robert Sesek
Addressed all comments.
10 years, 4 months ago (2010-08-17 21:25:30 UTC) #14
Mark Mentovai
10 years, 4 months ago (2010-08-17 21:26:43 UTC) #15
LG allays.

Powered by Google App Engine
This is Rietveld 408576698