Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
366029:
Initial change for the implementation of browser actions on the mac.... (Closed)
|
Created:
11 years, 1 month ago by Bons Modified:
9 years, 5 months ago CC:
chromium-reviews_googlegroups.com, ben+cc_chromium.org, John Grabowski, Erik does not do reviews, pam+watch_chromium.org, Paweł Hajdan Jr. Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionInitial change for the implementation of browser actions on the mac.
Popups are not implemented within this change.
BUG=23881
TEST=Install a browser action extension on the mac. Observe that something actually happens in the UI.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=31803
Patch Set 1 #
Total comments: 6
Patch Set 2 : '' #
Total comments: 38
Patch Set 3 : '' #
Total comments: 44
Patch Set 4 : '' #
Total comments: 6
Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : '' #
Messages
Total messages: 24 (0 generated)
Who is mmentovai@chromium.org? fixed.
Please include screen caps for UI changes.
Here is a screen cap Andrew sent me separately: http://dl.getdropbox.com/u/124107/browser_actions_osx.png
+cole Also, the badge text looks a little scrunched and maybe off center in that screen cap. Mark, is there another good font that you can suggest for that area (on Windows, we use Arial). Otherwise, maybe we should use Droid on mac? http://damieng.com/blog/2007/11/14/droid-font-family-courtesy-of-google-ascender http://codereview.chromium.org/366029/diff/1/8 File chrome/browser/cocoa/extensions/browser_actions_controller.mm (right): http://codereview.chromium.org/366029/diff/1/8#newcode89 Line 89: // A helper class to bridge the asyncronous Skia bitmap loading mechanism to typo: asynchronous http://codereview.chromium.org/366029/diff/1/8#newcode91 Line 91: class ExtensionImageTrackerBridge : public NotificationObserver, OOC, why is this extra level of indirection (ExtensionImageTrackerBridge) required when the other ports did not need it? http://codereview.chromium.org/366029/diff/1/3 File chrome/browser/cocoa/toolbar_controller.mm (right): http://codereview.chromium.org/366029/diff/1/3#newcode188 Line 188: // resizes, necesitating the probable resizing of surrounding elements typo: necessitating
OK. Ready for some more in-depth thrashing. Have at it. http://codereview.chromium.org/366029/diff/1/8 File chrome/browser/cocoa/extensions/browser_actions_controller.mm (right): http://codereview.chromium.org/366029/diff/1/8#newcode89 Line 89: // A helper class to bridge the asyncronous Skia bitmap loading mechanism to On 2009/11/05 20:13:15, Aaron Boodman wrote: > typo: asynchronous Done. http://codereview.chromium.org/366029/diff/1/8#newcode91 Line 91: class ExtensionImageTrackerBridge : public NotificationObserver, On 2009/11/05 20:13:15, Aaron Boodman wrote: > OOC, why is this extra level of indirection (ExtensionImageTrackerBridge) > required when the other ports did not need it? While you can freely mix Objective-C and C++ code within files such as this, you cannot directly inherit Objective-C objects from C++ ones. The object models are not directly compatible nor are the memory layouts. Since we need to inherit from a C++ class in this case, a bridge must be created, since the rest of the code uses Obj-C. http://codereview.chromium.org/366029/diff/1/3 File chrome/browser/cocoa/toolbar_controller.mm (right): http://codereview.chromium.org/366029/diff/1/3#newcode188 Line 188: // resizes, necesitating the probable resizing of surrounding elements On 2009/11/05 20:13:15, Aaron Boodman wrote: > typo: necessitating Done.
OK. Ready for some more in-depth thrashing. Have at it. http://codereview.chromium.org/366029/diff/1/8 File chrome/browser/cocoa/extensions/browser_actions_controller.mm (right): http://codereview.chromium.org/366029/diff/1/8#newcode89 Line 89: // A helper class to bridge the asyncronous Skia bitmap loading mechanism to On 2009/11/05 20:13:15, Aaron Boodman wrote: > typo: asynchronous Done. http://codereview.chromium.org/366029/diff/1/8#newcode91 Line 91: class ExtensionImageTrackerBridge : public NotificationObserver, On 2009/11/05 20:13:15, Aaron Boodman wrote: > OOC, why is this extra level of indirection (ExtensionImageTrackerBridge) > required when the other ports did not need it? While you can freely mix Objective-C and C++ code within files such as this, you cannot directly inherit Objective-C objects from C++ ones. The object models are not directly compatible nor are the memory layouts. Since we need to inherit from a C++ class in this case, a bridge must be created, since the rest of the code uses Obj-C. http://codereview.chromium.org/366029/diff/1/3 File chrome/browser/cocoa/toolbar_controller.mm (right): http://codereview.chromium.org/366029/diff/1/3#newcode188 Line 188: // resizes, necesitating the probable resizing of surrounding elements On 2009/11/05 20:13:15, Aaron Boodman wrote: > typo: necessitating Done. http://codereview.chromium.org/366029
The screen cap from earlier isn't quite right. See: http://dl.dropbox.com/u/124107/not_quite_right.png Have you fixed this? If so, please upload a new screen cap. Also, I think many macs have arial installed. Can we try that first, then fall back to helvetica?
The screen cap from earlier isn't quite right. See: http://dl.dropbox.com/u/124107/not_quite_right.png Have you fixed this? If so, please upload a new screen cap. Also, I think many macs have arial installed. Can we try that first, then fall back to helvetica? http://codereview.chromium.org/366029
On 2009/11/10 19:35:41, Aaron Boodman wrote: > The screen cap from earlier isn't quite right. See: > http://dl.dropbox.com/u/124107/not_quite_right.png > > Have you fixed this? If so, please upload a new screen cap. > > Also, I think many macs have arial installed. Can we try that first, then > fall > back to helvetica? > > http://codereview.chromium.org/366029 Working on it as part of fixing the layout issues described within the change description. Will update a screen cap when that changes.
On 2009/11/10 19:35:41, Aaron Boodman wrote: > The screen cap from earlier isn't quite right. See: > http://dl.dropbox.com/u/124107/not_quite_right.png > Have you fixed this? If so, please upload a new screen cap. > Also, I think many macs have arial installed. Can we try that first, then > fall > back to helvetica? > http://codereview.chromium.org/366029 Working on it as part of fixing the layout issues described within the change description. Will update a screen cap when that changes. http://codereview.chromium.org/366029
On 2009/11/10 19:35:23, Aaron Boodman wrote: > Also, I think many macs have arial installed. Can we try that first, then fall > back to helvetica? Note the inevitable whiners: I think we may ultimately go with Droid for this space on all platforms, but it doesn't look quite as nice as Arial at this size, and IMO, neither does Helvetica. So there!
On 2009/11/10 19:35:23, Aaron Boodman wrote: > Also, I think many macs have arial installed. Can we try that first, then > fall > back to helvetica? Note the inevitable whiners: I think we may ultimately go with Droid for this space on all platforms, but it doesn't look quite as nice as Arial at this size, and IMO, neither does Helvetica. So there! http://codereview.chromium.org/366029
COMMENTS! http://codereview.chromium.org/366029/diff/6001/7006 File chrome/browser/cocoa/extensions/browser_actions_controller.h (right): http://codereview.chromium.org/366029/diff/6001/7006#newcode1 Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved. svn propset svn:eol-style LF chrome/browser/cocoa/extensions/browser_actions_controller.h http://codereview.chromium.org/366029/diff/6001/7006#newcode10 Line 10: #include "base/scoped_nsobject.h" #import this one http://codereview.chromium.org/366029/diff/6001/7006#newcode48 Line 48: - (void)createButtons; What do I do? http://codereview.chromium.org/366029/diff/6001/7006#newcode50 Line 50: - (void)hidePopup; Me too. http://codereview.chromium.org/366029/diff/6001/7006#newcode52 Line 52: - (void)browserActionVisibilityHasChanged; … http://codereview.chromium.org/366029/diff/6001/7007 File chrome/browser/cocoa/extensions/browser_actions_controller.mm (right): http://codereview.chromium.org/366029/diff/6001/7007#newcode24 Line 24: // The current tab ID used when drawing the badge. @private http://codereview.chromium.org/366029/diff/6001/7007#newcode31 Line 31: @property(readwrite) int tabId; nonatomic? http://codereview.chromium.org/366029/diff/6001/7007#newcode32 Line 32: @property(readwrite) ExtensionAction* extensionAction; me too. http://codereview.chromium.org/366029/diff/6001/7007#newcode40 Line 40: -(void)drawRect:(NSRect)dirtyRect { minor nit: dash space (void) http://codereview.chromium.org/366029/diff/6001/7007#newcode147 Line 147: // Since the container is the maximum height of the toolbar, we have to move the Maybe you should collect these constants and the other one from above together and put them at the top of the file. http://codereview.chromium.org/366029/diff/6001/7007#newcode165 Line 165: if (![super initWithFrame:frame]) Is this functionally the same as the standard idiom? YES. But the standard idiom assigns into self. Because. I don't really know why. All I know is that one time I tried to do what you're doing here and pinkerton told me not to. http://codereview.chromium.org/366029/diff/6001/7007#newcode280 Line 280: break; Should this fall through to the NOTIMPLEMENTED for now? Or you can call to your hidePopup, which has the NOTIMPLEMENTED? http://codereview.chromium.org/366029/diff/6001/7007#newcode297 Line 297: extern const int kBrowserActionButtonPadding = 3; This const and the next one might want to live with the other constst too. http://codereview.chromium.org/366029/diff/6001/7007#newcode307 Line 307: if (![super init]) … http://codereview.chromium.org/366029/diff/6001/7007#newcode350 Line 350: [containerView_ setHidden:FALSE]; Did you mean: NO? http://codereview.chromium.org/366029/diff/6001/7007#newcode376 Line 376: [containerView_ setHidden:TRUE]; Did you mean: YES? (Look for these and fix them throughout.) http://codereview.chromium.org/366029/diff/6001/7002 File chrome/browser/cocoa/toolbar_controller.mm (right): http://codereview.chromium.org/366029/diff/6001/7002#newcode463 Line 463: // it doesn't cover all cases. OK? http://codereview.chromium.org/366029/diff/6001/7002#newcode465 Line 465: float width = ([browserActionsController_ buttonCount] * Don't use float. These are CGFloats, I think. Maybe your constants need to change types too. I'm not gonna check the other file again now though. Look for other spots where you used float… http://codereview.chromium.org/366029/diff/6001/7002#newcode468 Line 468: if (width < 0) { width = std::min(0, bleh);
OK. Screen caps are done... phew. http://andybons.com/chrome/badge-fonts/123%20-%20Arial.png http://andybons.com/chrome/badge-fonts/123%20-%20Droid%20Sans.png http://andybons.com/chrome/badge-fonts/123%20-%20Helvetica%20Bold.png http://andybons.com/chrome/badge-fonts/123%20-%20Helvetica.png http://andybons.com/chrome/badge-fonts/123%20-%20Lucida%20Grande.png http://andybons.com/chrome/badge-fonts/abg%20-%20Arial.png http://andybons.com/chrome/badge-fonts/abg%20-%20Droid%20Sans.png http://andybons.com/chrome/badge-fonts/abg%20-%20Helvetica%20Bold.png http://andybons.com/chrome/badge-fonts/abg%20-%20Helvetica.png http://andybons.com/chrome/badge-fonts/abg%20-%20Lucida%20Grande.png These are all 9 point since 8 was so unreadable. Still banging on the off-center look. Also, the offset of the badge is a bit higher because currently, if the bookmark bar is active, it will clip anything positioned lower in the toolbar. We can work on fixing that, but it's a can of worms I'd rather do in another change.
OK. Screen caps are done... phew. http://andybons.com/chrome/badge-fonts/123%20-%20Arial.png http://andybons.com/chrome/badge-fonts/123%20-%20Droid%20Sans.png http://andybons.com/chrome/badge-fonts/123%20-%20Helvetica%20Bold.png http://andybons.com/chrome/badge-fonts/123%20-%20Helvetica.png http://andybons.com/chrome/badge-fonts/123%20-%20Lucida%20Grande.png http://andybons.com/chrome/badge-fonts/abg%20-%20Arial.png http://andybons.com/chrome/badge-fonts/abg%20-%20Droid%20Sans.png http://andybons.com/chrome/badge-fonts/abg%20-%20Helvetica%20Bold.png http://andybons.com/chrome/badge-fonts/abg%20-%20Helvetica.png http://andybons.com/chrome/badge-fonts/abg%20-%20Lucida%20Grande.png These are all 9 point since 8 was so unreadable. Still banging on the off-center look. Also, the offset of the badge is a bit higher because currently, if the bookmark bar is active, it will clip anything positioned lower in the toolbar. We can work on fixing that, but it's a can of worms I'd rather do in another change. http://codereview.chromium.org/366029
Thanks for doing this. Given these grabs, I'll continue to advocate for using Droid for this. I think it looks good enough with "123" (although I dislike the '3') and better than the others at "abc". Given the symbol consistency and layout consistency that it would bring us across platforms, it seems like the right thing to do. Erik On Tue, Nov 10, 2009 at 1:34 PM, <andybons@chromium.org> wrote: > OK. Screen caps are done... phew. > > http://andybons.com/chrome/badge-fonts/123%20-%20Arial.png > http://andybons.com/chrome/badge-fonts/123%20-%20Droid%20Sans.png > http://andybons.com/chrome/badge-fonts/123%20-%20Helvetica%20Bold.png > http://andybons.com/chrome/badge-fonts/123%20-%20Helvetica.png > http://andybons.com/chrome/badge-fonts/123%20-%20Lucida%20Grande.png > http://andybons.com/chrome/badge-fonts/abg%20-%20Arial.png > http://andybons.com/chrome/badge-fonts/abg%20-%20Droid%20Sans.png > http://andybons.com/chrome/badge-fonts/abg%20-%20Helvetica%20Bold.png > http://andybons.com/chrome/badge-fonts/abg%20-%20Helvetica.png > http://andybons.com/chrome/badge-fonts/abg%20-%20Lucida%20Grande.png > > These are all 9 point since 8 was so unreadable. > > Still banging on the off-center look. > > Also, the offset of the badge is a bit higher because currently, if the > bookmark > bar is active, it will clip anything positioned lower in the toolbar. We > can > work on fixing that, but it's a can of worms I'd rather do in another > change. > > > > http://codereview.chromium.org/366029 >
OK. Take another look. Screen grabs are below (I've enabled borders just so you can see the spacing of the buttons more clearly. In the real interaction they wont have borders unless you mouse-over them). http://andybons.com/chrome/with_buttons.png http://andybons.com/chrome/without_buttons.png http://codereview.chromium.org/366029/diff/6001/7006 File chrome/browser/cocoa/extensions/browser_actions_controller.h (right): http://codereview.chromium.org/366029/diff/6001/7006#newcode1 Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved. On 2009/11/10 19:57:20, Mark Mentovai wrote: > svn propset svn:eol-style LF > chrome/browser/cocoa/extensions/browser_actions_controller.h Done. http://codereview.chromium.org/366029/diff/6001/7006#newcode10 Line 10: #include "base/scoped_nsobject.h" On 2009/11/10 19:57:20, Mark Mentovai wrote: > #import this one Done. http://codereview.chromium.org/366029/diff/6001/7006#newcode48 Line 48: - (void)createButtons; On 2009/11/10 19:57:20, Mark Mentovai wrote: > What do I do? Done. http://codereview.chromium.org/366029/diff/6001/7006#newcode50 Line 50: - (void)hidePopup; On 2009/11/10 19:57:20, Mark Mentovai wrote: > Me too. Done. http://codereview.chromium.org/366029/diff/6001/7006#newcode52 Line 52: - (void)browserActionVisibilityHasChanged; On 2009/11/10 19:57:20, Mark Mentovai wrote: > … Done. http://codereview.chromium.org/366029/diff/6001/7007 File chrome/browser/cocoa/extensions/browser_actions_controller.mm (right): http://codereview.chromium.org/366029/diff/6001/7007#newcode24 Line 24: // The current tab ID used when drawing the badge. On 2009/11/10 19:57:20, Mark Mentovai wrote: > @private Done. http://codereview.chromium.org/366029/diff/6001/7007#newcode31 Line 31: @property(readwrite) int tabId; On 2009/11/10 19:57:20, Mark Mentovai wrote: > nonatomic? I've seen both throughout the code. Do you suggest nonatomic for the speed difference? http://codereview.chromium.org/366029/diff/6001/7007#newcode32 Line 32: @property(readwrite) ExtensionAction* extensionAction; On 2009/11/10 19:57:20, Mark Mentovai wrote: > me too. Done. http://codereview.chromium.org/366029/diff/6001/7007#newcode40 Line 40: -(void)drawRect:(NSRect)dirtyRect { On 2009/11/10 19:57:20, Mark Mentovai wrote: > minor nit: dash space (void) Done. http://codereview.chromium.org/366029/diff/6001/7007#newcode147 Line 147: // Since the container is the maximum height of the toolbar, we have to move the On 2009/11/10 19:57:20, Mark Mentovai wrote: > Maybe you should collect these constants and the other one from above together > and put them at the top of the file. Done. http://codereview.chromium.org/366029/diff/6001/7007#newcode165 Line 165: if (![super initWithFrame:frame]) On 2009/11/10 19:57:20, Mark Mentovai wrote: > Is this functionally the same as the standard idiom? YES. But the standard > idiom assigns into self. Because. I don't really know why. All I know is that > one time I tried to do what you're doing here and pinkerton told me not to. Done. http://codereview.chromium.org/366029/diff/6001/7007#newcode280 Line 280: break; On 2009/11/10 19:57:20, Mark Mentovai wrote: > Should this fall through to the NOTIMPLEMENTED for now? > > Or you can call to your hidePopup, which has the NOTIMPLEMENTED? Done. http://codereview.chromium.org/366029/diff/6001/7007#newcode297 Line 297: extern const int kBrowserActionButtonPadding = 3; On 2009/11/10 19:57:20, Mark Mentovai wrote: > This const and the next one might want to live with the other constst too. Done. http://codereview.chromium.org/366029/diff/6001/7007#newcode307 Line 307: if (![super init]) On 2009/11/10 19:57:20, Mark Mentovai wrote: > > … Done. http://codereview.chromium.org/366029/diff/6001/7007#newcode350 Line 350: [containerView_ setHidden:FALSE]; On 2009/11/10 19:57:20, Mark Mentovai wrote: > Did you mean: NO? YES? :) http://codereview.chromium.org/366029/diff/6001/7007#newcode376 Line 376: [containerView_ setHidden:TRUE]; On 2009/11/10 19:57:20, Mark Mentovai wrote: > Did you mean: YES? > > (Look for these and fix them throughout.) Done. http://codereview.chromium.org/366029/diff/6001/7002 File chrome/browser/cocoa/toolbar_controller.mm (right): http://codereview.chromium.org/366029/diff/6001/7002#newcode463 Line 463: // it doesn't cover all cases. On 2009/11/10 19:57:20, Mark Mentovai wrote: > OK? Done. http://codereview.chromium.org/366029/diff/6001/7002#newcode465 Line 465: float width = ([browserActionsController_ buttonCount] * On 2009/11/10 19:57:20, Mark Mentovai wrote: > Don't use float. These are CGFloats, I think. Maybe your constants need to > change types too. I'm not gonna check the other file again now though. Look > for other spots where you used float… Done. http://codereview.chromium.org/366029/diff/6001/7002#newcode468 Line 468: if (width < 0) { On 2009/11/10 19:57:20, Mark Mentovai wrote: > width = std::min(0, bleh); Done.
It's not likely that I'll be able to take another close look at this today, there's way too much going on. If you can wait, cool. Otherwise, I can try to help you find a substitute. Mark andybons@chromium.org wrote: > OK. Take another look. Screen grabs are below (I've enabled borders just = so > you > can see the spacing of the buttons more clearly. In the real interaction > they > wont have borders unless you mouse-over them). > > http://andybons.com/chrome/with_buttons.png > http://andybons.com/chrome/without_buttons.png > > > http://codereview.chromium.org/366029/diff/6001/7006 > File chrome/browser/cocoa/extensions/browser_actions_controller.h > (right): > > http://codereview.chromium.org/366029/diff/6001/7006#newcode1 > Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved. > On 2009/11/10 19:57:20, Mark Mentovai wrote: >> >> svn propset svn:eol-style LF >> chrome/browser/cocoa/extensions/browser_actions_controller.h > > Done. > > http://codereview.chromium.org/366029/diff/6001/7006#newcode10 > Line 10: #include "base/scoped_nsobject.h" > On 2009/11/10 19:57:20, Mark Mentovai wrote: >> >> #import this one > > Done. > > http://codereview.chromium.org/366029/diff/6001/7006#newcode48 > Line 48: - (void)createButtons; > On 2009/11/10 19:57:20, Mark Mentovai wrote: >> >> What do I do? > > Done. > > http://codereview.chromium.org/366029/diff/6001/7006#newcode50 > Line 50: - (void)hidePopup; > On 2009/11/10 19:57:20, Mark Mentovai wrote: >> >> Me too. > > Done. > > http://codereview.chromium.org/366029/diff/6001/7006#newcode52 > Line 52: - (void)browserActionVisibilityHasChanged; > On 2009/11/10 19:57:20, Mark Mentovai wrote: >> >> =85 > > Done. > > http://codereview.chromium.org/366029/diff/6001/7007 > File chrome/browser/cocoa/extensions/browser_actions_controller.mm > (right): > > http://codereview.chromium.org/366029/diff/6001/7007#newcode24 > Line 24: // The current tab ID used when drawing the badge. > On 2009/11/10 19:57:20, Mark Mentovai wrote: >> >> @private > > Done. > > http://codereview.chromium.org/366029/diff/6001/7007#newcode31 > Line 31: @property(readwrite) int tabId; > On 2009/11/10 19:57:20, Mark Mentovai wrote: >> >> nonatomic? > > I've seen both throughout the code. Do you suggest nonatomic for the > speed difference? > > http://codereview.chromium.org/366029/diff/6001/7007#newcode32 > Line 32: @property(readwrite) ExtensionAction* extensionAction; > On 2009/11/10 19:57:20, Mark Mentovai wrote: >> >> me too. > > Done. > > http://codereview.chromium.org/366029/diff/6001/7007#newcode40 > Line 40: -(void)drawRect:(NSRect)dirtyRect { > On 2009/11/10 19:57:20, Mark Mentovai wrote: >> >> minor nit: dash space (void) > > Done. > > http://codereview.chromium.org/366029/diff/6001/7007#newcode147 > Line 147: // Since the container is the maximum height of the toolbar, > we have to move the > On 2009/11/10 19:57:20, Mark Mentovai wrote: >> >> Maybe you should collect these constants and the other one from above > > together >> >> and put them at the top of the file. > > Done. > > http://codereview.chromium.org/366029/diff/6001/7007#newcode165 > Line 165: if (![super initWithFrame:frame]) > On 2009/11/10 19:57:20, Mark Mentovai wrote: >> >> Is this functionally the same as the standard idiom? =A0YES. =A0But the > > standard >> >> idiom assigns into self. =A0Because. =A0I don't really know why. =A0All = I > > know is that >> >> one time I tried to do what you're doing here and pinkerton told me > > not to. > > Done. > > http://codereview.chromium.org/366029/diff/6001/7007#newcode280 > Line 280: break; > On 2009/11/10 19:57:20, Mark Mentovai wrote: >> >> Should this fall through to the NOTIMPLEMENTED for now? > >> Or you can call to your hidePopup, which has the NOTIMPLEMENTED? > > Done. > > http://codereview.chromium.org/366029/diff/6001/7007#newcode297 > Line 297: extern const int kBrowserActionButtonPadding =3D 3; > On 2009/11/10 19:57:20, Mark Mentovai wrote: >> >> This const and the next one might want to live with the other constst > > too. > > Done. > > http://codereview.chromium.org/366029/diff/6001/7007#newcode307 > Line 307: if (![super init]) > On 2009/11/10 19:57:20, Mark Mentovai wrote: > >> =85 > > Done. > > http://codereview.chromium.org/366029/diff/6001/7007#newcode350 > Line 350: [containerView_ setHidden:FALSE]; > On 2009/11/10 19:57:20, Mark Mentovai wrote: >> >> Did you mean: NO? > > YES? :) > > http://codereview.chromium.org/366029/diff/6001/7007#newcode376 > Line 376: [containerView_ setHidden:TRUE]; > On 2009/11/10 19:57:20, Mark Mentovai wrote: >> >> Did you mean: YES? > >> (Look for these and fix them throughout.) > > Done. > > http://codereview.chromium.org/366029/diff/6001/7002 > File chrome/browser/cocoa/toolbar_controller.mm (right): > > http://codereview.chromium.org/366029/diff/6001/7002#newcode463 > Line 463: // it doesn't cover all cases. > On 2009/11/10 19:57:20, Mark Mentovai wrote: >> >> OK? > > Done. > > http://codereview.chromium.org/366029/diff/6001/7002#newcode465 > Line 465: float width =3D ([browserActionsController_ buttonCount] * > On 2009/11/10 19:57:20, Mark Mentovai wrote: >> >> Don't use float. =A0These are CGFloats, I think. =A0Maybe your constants > > need to >> >> change types too. =A0I'm not gonna check the other file again now > > though. =A0Look >> >> for other spots where you used float=85 > > Done. > > http://codereview.chromium.org/366029/diff/6001/7002#newcode468 > Line 468: if (width < 0) { > On 2009/11/10 19:57:20, Mark Mentovai wrote: >> >> width =3D std::min(0, bleh); > > Done. > > http://codereview.chromium.org/366029 >
No worries. I will be looking into the QT plugin problem and implementing the browser action popups in another CL. It's not blocking. A On Wed, Nov 11, 2009 at 3:42 PM, Mark Mentovai <mark@chromium.org> wrote: > It's not likely that I'll be able to take another close look at this > today, there's way too much going on. =C2=A0If you can wait, cool. > Otherwise, I can try to help you find a substitute. > > Mark > > andybons@chromium.org wrote: >> OK. Take another look. Screen grabs are below (I've enabled borders just= so >> you >> can see the spacing of the buttons more clearly. In the real interaction >> they >> wont have borders unless you mouse-over them). >> >> http://andybons.com/chrome/with_buttons.png >> http://andybons.com/chrome/without_buttons.png >> >> >> http://codereview.chromium.org/366029/diff/6001/7006 >> File chrome/browser/cocoa/extensions/browser_actions_controller.h >> (right): >> >> http://codereview.chromium.org/366029/diff/6001/7006#newcode1 >> Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved. >> On 2009/11/10 19:57:20, Mark Mentovai wrote: >>> >>> svn propset svn:eol-style LF >>> chrome/browser/cocoa/extensions/browser_actions_controller.h >> >> Done. >> >> http://codereview.chromium.org/366029/diff/6001/7006#newcode10 >> Line 10: #include "base/scoped_nsobject.h" >> On 2009/11/10 19:57:20, Mark Mentovai wrote: >>> >>> #import this one >> >> Done. >> >> http://codereview.chromium.org/366029/diff/6001/7006#newcode48 >> Line 48: - (void)createButtons; >> On 2009/11/10 19:57:20, Mark Mentovai wrote: >>> >>> What do I do? >> >> Done. >> >> http://codereview.chromium.org/366029/diff/6001/7006#newcode50 >> Line 50: - (void)hidePopup; >> On 2009/11/10 19:57:20, Mark Mentovai wrote: >>> >>> Me too. >> >> Done. >> >> http://codereview.chromium.org/366029/diff/6001/7006#newcode52 >> Line 52: - (void)browserActionVisibilityHasChanged; >> On 2009/11/10 19:57:20, Mark Mentovai wrote: >>> >>> =E2=80=A6 >> >> Done. >> >> http://codereview.chromium.org/366029/diff/6001/7007 >> File chrome/browser/cocoa/extensions/browser_actions_controller.mm >> (right): >> >> http://codereview.chromium.org/366029/diff/6001/7007#newcode24 >> Line 24: // The current tab ID used when drawing the badge. >> On 2009/11/10 19:57:20, Mark Mentovai wrote: >>> >>> @private >> >> Done. >> >> http://codereview.chromium.org/366029/diff/6001/7007#newcode31 >> Line 31: @property(readwrite) int tabId; >> On 2009/11/10 19:57:20, Mark Mentovai wrote: >>> >>> nonatomic? >> >> I've seen both throughout the code. Do you suggest nonatomic for the >> speed difference? >> >> http://codereview.chromium.org/366029/diff/6001/7007#newcode32 >> Line 32: @property(readwrite) ExtensionAction* extensionAction; >> On 2009/11/10 19:57:20, Mark Mentovai wrote: >>> >>> me too. >> >> Done. >> >> http://codereview.chromium.org/366029/diff/6001/7007#newcode40 >> Line 40: -(void)drawRect:(NSRect)dirtyRect { >> On 2009/11/10 19:57:20, Mark Mentovai wrote: >>> >>> minor nit: dash space (void) >> >> Done. >> >> http://codereview.chromium.org/366029/diff/6001/7007#newcode147 >> Line 147: // Since the container is the maximum height of the toolbar, >> we have to move the >> On 2009/11/10 19:57:20, Mark Mentovai wrote: >>> >>> Maybe you should collect these constants and the other one from above >> >> together >>> >>> and put them at the top of the file. >> >> Done. >> >> http://codereview.chromium.org/366029/diff/6001/7007#newcode165 >> Line 165: if (![super initWithFrame:frame]) >> On 2009/11/10 19:57:20, Mark Mentovai wrote: >>> >>> Is this functionally the same as the standard idiom? =C2=A0YES. =C2=A0B= ut the >> >> standard >>> >>> idiom assigns into self. =C2=A0Because. =C2=A0I don't really know why. = =C2=A0All I >> >> know is that >>> >>> one time I tried to do what you're doing here and pinkerton told me >> >> not to. >> >> Done. >> >> http://codereview.chromium.org/366029/diff/6001/7007#newcode280 >> Line 280: break; >> On 2009/11/10 19:57:20, Mark Mentovai wrote: >>> >>> Should this fall through to the NOTIMPLEMENTED for now? >> >>> Or you can call to your hidePopup, which has the NOTIMPLEMENTED? >> >> Done. >> >> http://codereview.chromium.org/366029/diff/6001/7007#newcode297 >> Line 297: extern const int kBrowserActionButtonPadding =3D 3; >> On 2009/11/10 19:57:20, Mark Mentovai wrote: >>> >>> This const and the next one might want to live with the other constst >> >> too. >> >> Done. >> >> http://codereview.chromium.org/366029/diff/6001/7007#newcode307 >> Line 307: if (![super init]) >> On 2009/11/10 19:57:20, Mark Mentovai wrote: >> >>> =E2=80=A6 >> >> Done. >> >> http://codereview.chromium.org/366029/diff/6001/7007#newcode350 >> Line 350: [containerView_ setHidden:FALSE]; >> On 2009/11/10 19:57:20, Mark Mentovai wrote: >>> >>> Did you mean: NO? >> >> YES? :) >> >> http://codereview.chromium.org/366029/diff/6001/7007#newcode376 >> Line 376: [containerView_ setHidden:TRUE]; >> On 2009/11/10 19:57:20, Mark Mentovai wrote: >>> >>> Did you mean: YES? >> >>> (Look for these and fix them throughout.) >> >> Done. >> >> http://codereview.chromium.org/366029/diff/6001/7002 >> File chrome/browser/cocoa/toolbar_controller.mm (right): >> >> http://codereview.chromium.org/366029/diff/6001/7002#newcode463 >> Line 463: // it doesn't cover all cases. >> On 2009/11/10 19:57:20, Mark Mentovai wrote: >>> >>> OK? >> >> Done. >> >> http://codereview.chromium.org/366029/diff/6001/7002#newcode465 >> Line 465: float width =3D ([browserActionsController_ buttonCount] * >> On 2009/11/10 19:57:20, Mark Mentovai wrote: >>> >>> Don't use float. =C2=A0These are CGFloats, I think. =C2=A0Maybe your co= nstants >> >> need to >>> >>> change types too. =C2=A0I'm not gonna check the other file again now >> >> though. =C2=A0Look >>> >>> for other spots where you used float=E2=80=A6 >> >> Done. >> >> http://codereview.chromium.org/366029/diff/6001/7002#newcode468 >> Line 468: if (width < 0) { >> On 2009/11/10 19:57:20, Mark Mentovai wrote: >>> >>> width =3D std::min(0, bleh); >> >> Done. >> >> http://codereview.chromium.org/366029 >> >
Nice job. http://codereview.chromium.org/366029/diff/11001/11007 File chrome/browser/cocoa/extensions/browser_actions_controller.h (right): http://codereview.chromium.org/366029/diff/11001/11007#newcode38 Line 38: // container view. What are the keys? You call them button keys over in the other file and in the next comment. extension.h calls them IDs. That's why I asked you where to find extension.h. I think we should be consistent in naming. Also, you should say what you're using for keys here. What are the values? BrowserActionButton. Say so. http://codereview.chromium.org/366029/diff/11001/11007#newcode66 Line 66: // BrowserActionButton. Instead of documenting "|sender| is a BrowserActionButton", can you make the argument a BrowserActionButton*? Maybe because you meant BrowserActionsController*? http://codereview.chromium.org/366029/diff/11001/11007#newcode69 Line 69: // Returns the current ID of the active tab, -1 otherwise. What's "otherwise" mean? http://codereview.chromium.org/366029/diff/11001/11008 File chrome/browser/cocoa/extensions/browser_actions_controller.mm (right): http://codereview.chromium.org/366029/diff/11001/11008#newcode24 Line 24: // Since the container is the maximum height of the toolbar, we have to move the Blank before so it's obvious what each comment applies to. Throughout this block. http://codereview.chromium.org/366029/diff/11001/11008#newcode33 Line 33: NSString* const kBrowserActionsChangedNotification = @"BrowserActionsChanged"; And a blank before this guy so that it doesn't blend into the "the padding between" thing above. http://codereview.chromium.org/366029/diff/11001/11008#newcode36 Line 36: @private 1-space indent. http://codereview.chromium.org/366029/diff/11001/11008#newcode68 Line 68: @class ExtensionImageTrackerBridge; This is a C++ class, so use |class| and not |@class|. http://codereview.chromium.org/366029/diff/11001/11008#newcode71 Line 71: @private 1-space indent. http://codereview.chromium.org/366029/diff/11001/11008#newcode150 Line 150: // because destructors of refcounted objects are private. This comment doesn't really tell me anything about the memory model here. It has a private destructor so we can't put it in a scoped_ptr. Big whoop. That only leaves me asking "why doesn't this class hold a ref then?" I looked at image_loading_tracker.h and I found the answer, but I shouldn't have had to have done that to assure myself that there's no leak. The comment doesn't need to be long and wordy, it just needs to say something that tells the reader about the ownership relationship. http://codereview.chromium.org/366029/diff/11001/11008#newcode173 Line 173: // [NSButton setCell:] warns to NOT use setCell: other than in the This is a great comment, thanks for including it. http://codereview.chromium.org/366029/diff/11001/11008#newcode201 Line 201: defaultIcon_.reset([image retain]); Awesome, correct use of retain with a scoped_nsobject. http://codereview.chromium.org/366029/diff/11001/11008#newcode240 Line 240: @end // @interface BrowserActionsController(Private) I wrote a comment here ten minutes ago. Now I see Rietveld ate it. I ♥ Rietveld. I said something like: This @interface is only three lines long, so it's the one in here that probably needs the ending comment least of all. If you want to use these comments, go ahead and use them on all of the other @ends in this file. Otherwise, take this one out (and maybe the one in the header too). Consistency. It's what's for dinner. http://codereview.chromium.org/366029/diff/11001/11008#newcode313 Line 313: buttons_.reset([[NSMutableDictionary alloc] initWithCapacity:1]); I wonder why you used initWithCapacity:1 on this and the array. You can just use [[NSJunk alloc] init]; for both. http://codereview.chromium.org/366029/diff/11001/11008#newcode363 Line 363: postNotificationName:kBrowserActionsChangedNotification object:nil]; It's customary to set object to the sender, like self. Line 379 too. http://codereview.chromium.org/366029/diff/11001/11008#newcode376 Line 376: [self repositionActionButtons]; I wonder why you call this in -removeActionButtonForExtension: but not -createActionButtonForExtension:. Maybe it would be obvious if -repositionActionButtons had a comment in the @interface. Or even in the implementation. You mean you expect me to actually read that code? UGH. OK, fine, I get it now. It's actually pretty obvious, but a one-liner @interface comment might be nice, "does x when a button is removed." http://codereview.chromium.org/366029/diff/11001/11008#newcode386 Line 386: [buttons_ objectForKey:[buttonOrder_ objectAtIndex:i]]; If buttonOrder_ stored BrowserActionButtons and not NSString IDs ("keys," whatever), then you wouldn't need to do an extra dictionary lookup here. You're already looking up [buttons_ objectForKey:buttonKey] in -removeActionButtonForExtension, so you've already got easy access to what you need to give to -removeObject: up in that function. Seems like a win to me. http://codereview.chromium.org/366029/diff/11001/11008#newcode401 Line 401: // Popups are not implemented for mac yet. Capital M on your platform of choice? http://codereview.chromium.org/366029/diff/11001/11003 File chrome/browser/cocoa/toolbar_controller.mm (right): http://codereview.chromium.org/366029/diff/11001/11003#newcode190 Line 190: // resizes, necessitating the probable resizing of surrounding elements is resized, necessitating http://codereview.chromium.org/366029/diff/11001/11003#newcode192 Line 192: [[NSNotificationCenter defaultCenter] When do you remove this observer? (Leak!) http://codereview.chromium.org/366029/diff/11001/11003#newcode440 Line 440: float moveX = 2 * [self interButtonSpacing] + NSWidth([pageButton_ frame]) + Not float, but what? http://codereview.chromium.org/366029/diff/11001/11003#newcode474 Line 474: width = std::max((CGFloat)0, width); I might have suggested std::max when I looked before, but now that I'm reading this closely, I think that we don't really want "max" or "if it's less than zero," we want to say "if there aren't any buttons." Do you agree? CGFloat width = 0.0; if (buttonCount) { width = /* math is hard */ } Does that convey intent better? http://codereview.chromium.org/366029/diff/11001/11003#newcode482 Line 482: // If a button is being added. Add spacing inward by negating the value. Shouldn't that be added comma add? http://codereview.chromium.org/366029/diff/11001/11003#newcode488 Line 488: if ((buttonCount == 1 && addingButton) || buttonCount == 0 && !addingButton) { Is anyone ever 100% positive of how (a && b || c) will be evaluated? I'd toss another pair of parentheses on this one for clarity, even though your code is correct. (Technically, you didn't need any inner parentheses at all.)
http://codereview.chromium.org/366029/diff/11001/11007 File chrome/browser/cocoa/extensions/browser_actions_controller.h (right): http://codereview.chromium.org/366029/diff/11001/11007#newcode38 Line 38: // container view. On 2009/11/12 06:18:52, Mark Mentovai wrote: > What are the keys? You call them button keys over in the other file and in the > next comment. extension.h calls them IDs. That's why I asked you where to find > extension.h. I think we should be consistent in naming. Also, you should say > what you're using for keys here. > > What are the values? BrowserActionButton. Say so. Done. http://codereview.chromium.org/366029/diff/11001/11007#newcode66 Line 66: // BrowserActionButton. On 2009/11/12 06:18:52, Mark Mentovai wrote: > Instead of documenting "|sender| is a BrowserActionButton", can you make the > argument a BrowserActionButton*? > > Maybe because you meant BrowserActionsController*? Done. http://codereview.chromium.org/366029/diff/11001/11007#newcode69 Line 69: // Returns the current ID of the active tab, -1 otherwise. On 2009/11/12 06:18:52, Mark Mentovai wrote: > What's "otherwise" mean? Done. http://codereview.chromium.org/366029/diff/11001/11008 File chrome/browser/cocoa/extensions/browser_actions_controller.mm (right): http://codereview.chromium.org/366029/diff/11001/11008#newcode24 Line 24: // Since the container is the maximum height of the toolbar, we have to move the On 2009/11/12 06:18:52, Mark Mentovai wrote: > Blank before so it's obvious what each comment applies to. Throughout this > block. Done. http://codereview.chromium.org/366029/diff/11001/11008#newcode33 Line 33: NSString* const kBrowserActionsChangedNotification = @"BrowserActionsChanged"; On 2009/11/12 06:18:52, Mark Mentovai wrote: > And a blank before this guy so that it doesn't blend into the "the padding > between" thing above. Done. http://codereview.chromium.org/366029/diff/11001/11008#newcode36 Line 36: @private On 2009/11/12 06:18:52, Mark Mentovai wrote: > 1-space indent. Done. http://codereview.chromium.org/366029/diff/11001/11008#newcode68 Line 68: @class ExtensionImageTrackerBridge; On 2009/11/12 06:18:52, Mark Mentovai wrote: > This is a C++ class, so use |class| and not |@class|. Done. http://codereview.chromium.org/366029/diff/11001/11008#newcode71 Line 71: @private On 2009/11/12 06:18:52, Mark Mentovai wrote: > 1-space indent. Done. http://codereview.chromium.org/366029/diff/11001/11008#newcode150 Line 150: // because destructors of refcounted objects are private. On 2009/11/12 06:18:52, Mark Mentovai wrote: > This comment doesn't really tell me anything about the memory model here. It > has a private destructor so we can't put it in a scoped_ptr. Big whoop. That > only leaves me asking "why doesn't this class hold a ref then?" I looked at > image_loading_tracker.h and I found the answer, but I shouldn't have had to have > done that to assure myself that there's no leak. > > The comment doesn't need to be long and wordy, it just needs to say something > that tells the reader about the ownership relationship. Done. http://codereview.chromium.org/366029/diff/11001/11008#newcode240 Line 240: @end // @interface BrowserActionsController(Private) On 2009/11/12 06:18:52, Mark Mentovai wrote: > I wrote a comment here ten minutes ago. Now I see Rietveld ate it. I ♥ > Rietveld. > > I said something like: > > This @interface is only three lines long, so it's the one in here that probably > needs the ending comment least of all. If you want to use these comments, go > ahead and use them on all of the other @ends in this file. Otherwise, take this > one out (and maybe the one in the header too). > > Consistency. It's what's for dinner. Done. http://codereview.chromium.org/366029/diff/11001/11008#newcode313 Line 313: buttons_.reset([[NSMutableDictionary alloc] initWithCapacity:1]); On 2009/11/12 06:18:52, Mark Mentovai wrote: > I wonder why you used initWithCapacity:1 on this and the array. > > You can just use [[NSJunk alloc] init]; for both. Done. http://codereview.chromium.org/366029/diff/11001/11008#newcode363 Line 363: postNotificationName:kBrowserActionsChangedNotification object:nil]; On 2009/11/12 06:18:52, Mark Mentovai wrote: > It's customary to set object to the sender, like self. Line 379 too. Done. http://codereview.chromium.org/366029/diff/11001/11008#newcode376 Line 376: [self repositionActionButtons]; On 2009/11/12 06:18:52, Mark Mentovai wrote: > I wonder why you call this in -removeActionButtonForExtension: but not > -createActionButtonForExtension:. Maybe it would be obvious if > -repositionActionButtons had a comment in the @interface. Or even in the > implementation. > > You mean you expect me to actually read that code? UGH. > > OK, fine, I get it now. It's actually pretty obvious, but a one-liner > @interface comment might be nice, "does x when a button is removed." Done. http://codereview.chromium.org/366029/diff/11001/11008#newcode386 Line 386: [buttons_ objectForKey:[buttonOrder_ objectAtIndex:i]]; On 2009/11/12 06:18:52, Mark Mentovai wrote: > If buttonOrder_ stored BrowserActionButtons and not NSString IDs ("keys," > whatever), then you wouldn't need to do an extra dictionary lookup here. > > You're already looking up [buttons_ objectForKey:buttonKey] in > -removeActionButtonForExtension, so you've already got easy access to what you > need to give to -removeObject: up in that function. Seems like a win to me. Done. http://codereview.chromium.org/366029/diff/11001/11008#newcode401 Line 401: // Popups are not implemented for mac yet. On 2009/11/12 06:18:52, Mark Mentovai wrote: > Capital M on your platform of choice? Done. http://codereview.chromium.org/366029/diff/11001/11003 File chrome/browser/cocoa/toolbar_controller.mm (right): http://codereview.chromium.org/366029/diff/11001/11003#newcode190 Line 190: // resizes, necessitating the probable resizing of surrounding elements On 2009/11/12 06:18:52, Mark Mentovai wrote: > is resized, necessitating Done. http://codereview.chromium.org/366029/diff/11001/11003#newcode192 Line 192: [[NSNotificationCenter defaultCenter] On 2009/11/12 06:18:52, Mark Mentovai wrote: > When do you remove this observer? (Leak!) Done. http://codereview.chromium.org/366029/diff/11001/11003#newcode440 Line 440: float moveX = 2 * [self interButtonSpacing] + NSWidth([pageButton_ frame]) + On 2009/11/12 06:18:52, Mark Mentovai wrote: > Not float, but what? Done. http://codereview.chromium.org/366029/diff/11001/11003#newcode474 Line 474: width = std::max((CGFloat)0, width); On 2009/11/12 06:18:52, Mark Mentovai wrote: > I might have suggested std::max when I looked before, but now that I'm reading > this closely, I think that we don't really want "max" or "if it's less than > zero," we want to say "if there aren't any buttons." Do you agree? > > CGFloat width = 0.0; > if (buttonCount) { > width = /* math is hard */ > } > > Does that convey intent better? Done. http://codereview.chromium.org/366029/diff/11001/11003#newcode482 Line 482: // If a button is being added. Add spacing inward by negating the value. On 2009/11/12 06:18:52, Mark Mentovai wrote: > Shouldn't that be added comma add? Done. http://codereview.chromium.org/366029/diff/11001/11003#newcode488 Line 488: if ((buttonCount == 1 && addingButton) || buttonCount == 0 && !addingButton) { On 2009/11/12 06:18:52, Mark Mentovai wrote: > Is anyone ever 100% positive of how (a && b || c) will be evaluated? I'd toss > another pair of parentheses on this one for clarity, even though your code is > correct. (Technically, you didn't need any inner parentheses at all.) Done.
LGTM http://codereview.chromium.org/366029/diff/14001/14007 File chrome/browser/cocoa/extensions/browser_actions_controller.h (right): http://codereview.chromium.org/366029/diff/14001/14007#newcode43 Line 43: // The order of the button keys within the dictionary. This contains BrowserActionButtons now, not keys, right? http://codereview.chromium.org/366029/diff/14001/14008 File chrome/browser/cocoa/extensions/browser_actions_controller.mm (right): http://codereview.chromium.org/366029/diff/14001/14008#newcode167 Line 167: NSRect frame = NSMakeRect( You can do this as written here, but I think that these are more readable written as: NSRect frame = NSMakeRect(xOffset, kBrowserActionOriginYOffset, kBrowserActionWidth, kBrowserActionHeight); http://codereview.chromium.org/366029/diff/14001/14003 File chrome/browser/cocoa/toolbar_controller.mm (right): http://codereview.chromium.org/366029/diff/14001/14003#newcode7 Line 7: #include <algorithm> No longer needed, right?
http://codereview.chromium.org/366029/diff/14001/14007 File chrome/browser/cocoa/extensions/browser_actions_controller.h (right): http://codereview.chromium.org/366029/diff/14001/14007#newcode43 Line 43: // The order of the button keys within the dictionary. On 2009/11/12 15:10:15, Mark Mentovai wrote: > This contains BrowserActionButtons now, not keys, right? Done. http://codereview.chromium.org/366029/diff/14001/14008 File chrome/browser/cocoa/extensions/browser_actions_controller.mm (right): http://codereview.chromium.org/366029/diff/14001/14008#newcode167 Line 167: NSRect frame = NSMakeRect( On 2009/11/12 15:10:15, Mark Mentovai wrote: > You can do this as written here, but I think that these are more readable > written as: > > NSRect frame = NSMakeRect(xOffset, > kBrowserActionOriginYOffset, > kBrowserActionWidth, > kBrowserActionHeight); Done. http://codereview.chromium.org/366029/diff/14001/14003 File chrome/browser/cocoa/toolbar_controller.mm (right): http://codereview.chromium.org/366029/diff/14001/14003#newcode7 Line 7: #include <algorithm> On 2009/11/12 15:10:15, Mark Mentovai wrote: > No longer needed, right? Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
