Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10533086:
Action box menu (Closed)
|
Created:
8 years, 6 months ago by yefimt Modified:
8 years, 4 months ago CC:
chromium-reviews, mihaip-chromium-reviews_chromium.org, tfarina, rudygalfi Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionInitial implementation of action box menu. It is still work in progress, just CL is getting too big
and need to share existing code with other developers.
BUG=125307
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150869
Patch Set 1 #
Total comments: 38
Patch Set 2 : Action box menu #Patch Set 3 : Action box menu #Patch Set 4 : Action box menu #
Total comments: 93
Patch Set 5 : Action box menu #Patch Set 6 : Action box menu #Patch Set 7 : Action box menu #
Total comments: 76
Patch Set 8 : Action box menu #
Total comments: 68
Patch Set 9 : Action box menu #
Total comments: 2
Patch Set 10 : Action box menu #Patch Set 11 : Action box menu #Patch Set 12 : Action box menu #
Total comments: 63
Patch Set 13 : Action box menu #Patch Set 14 : Action box menu #
Total comments: 138
Patch Set 15 : Action box menu #
Total comments: 68
Patch Set 16 : Action box menu #Patch Set 17 : Action box menu #
Total comments: 2
Patch Set 18 : Action box menu #
Total comments: 42
Patch Set 19 : Action box menu #Patch Set 20 : Action box menu #
Total comments: 1
Patch Set 21 : Action box menu #Patch Set 22 : Action box menu #Messages
Total messages: 62 (0 generated)
seems like a reasonable approach overall http://codereview.chromium.org/10533086/diff/1/chrome/browser/extensions/exte... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/10533086/diff/1/chrome/browser/extensions/exte... chrome/browser/extensions/extension_prefs.cc:72: const char kExtensionActionbox[] = "extensions.actionbox"; I would recommend: extensions.action_box_order http://codereview.chromium.org/10533086/diff/1/chrome/browser/extensions/exte... chrome/browser/extensions/extension_prefs.cc:74: const char kExtensionActionboxBar[] = "extensions.actionbox_bar"; And: toolbar_order_for_action_box_mode http://codereview.chromium.org/10533086/diff/1/chrome/browser/extensions/exte... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/10533086/diff/1/chrome/browser/extensions/exte... chrome/browser/extensions/extension_prefs.h:112: std::vector<std::string> GetActionboxOrder(); Nit: I think we should use 'ActionBox', not 'Actionbox'. http://codereview.chromium.org/10533086/diff/1/chrome/browser/extensions/exte... File chrome/browser/extensions/extension_toolbar_model.cc (right): http://codereview.chromium.org/10533086/diff/1/chrome/browser/extensions/exte... chrome/browser/extensions/extension_toolbar_model.cc:204: void ExtensionToolbarModel::InitializeExtensionLists() { This is pretty hard to follow. How about something like: void Populate() { if (IsActionBoxEnabled()) PopulateForActionBoxMode(); else PopulateForNonActionBoxMode(); } void PopulateForActionBoxMode() { // all enabled extensions that have browser actions are shown. // there's no way to hide browser actions in action box mode. // if an extension is explicitly in the toolbar, put it in // the toolbar list. otherwise, it goes in the action box list. } void PopulateToolbarForActionBoxMode() { // old populate logic here. } This would mean undoing some of the visibility changes you did earlier, but it seems simpler this way. http://codereview.chromium.org/10533086/diff/1/chrome/browser/extensions/exte... File chrome/browser/extensions/extension_toolbar_model.h (right): http://codereview.chromium.org/10533086/diff/1/chrome/browser/extensions/exte... chrome/browser/extensions/extension_toolbar_model.h:99: const extensions::Extension* GetActionBoxExtensionByIndex(int index) const; Why not just expose the ExtensionList directly? Consider changing the toolbar one to do the same. http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/toolbar/acti... File chrome/browser/ui/toolbar/action_box_menu_model.h (right): http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/toolbar/acti... chrome/browser/ui/toolbar/action_box_menu_model.h:31: // Overridden for both ButtonMenuItemModel::Delegate and SimpleMenuModel: If these aren't going to be called except through the interfaces, make them private. http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/toolbar/acti... chrome/browser/ui/toolbar/action_box_menu_model.h:45: size_t action_box_extensions_size(); This helper doesn't seem necessary. Can just get the value from the model directly when needed. http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/action... File chrome/browser/ui/views/action_box_menu.cc (right): http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/action... chrome/browser/ui/views/action_box_menu.cc:45: int id = 1; This is never used for anything...? Seems like it could be defined inside PopulateMenu(). http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/action... File chrome/browser/ui/views/action_box_menu.h (right): http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/action... chrome/browser/ui/views/action_box_menu.h:39: void Init(); Make private. http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/action... chrome/browser/ui/views/action_box_menu.h:44: // BrowserActionsHostDelegate overrides: Make these private. http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/action... File chrome/browser/ui/views/action_box_menu_item_view.cc (right): http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/action... chrome/browser/ui/views/action_box_menu_item_view.cc:14: MenuItemView::Layout(); I think the child layout is unnecessary since we're doing our own custom layout anyway. http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/action... chrome/browser/ui/views/action_box_menu_item_view.cc:19: gfx::Size size = child->GetPreferredSize(); child->SetPosition(Position(0, 0)); child->SizeToPreferredSize(); http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/action... chrome/browser/ui/views/action_box_menu_item_view.cc:33: MenuItemView::OnPaint(canvas); No need to override? http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/browse... File chrome/browser/ui/views/browser_action_view.h (right): http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/browse... chrome/browser/ui/views/browser_action_view.h:117: BrowserActionsHostDelegate* panel_; s/panel_/delegate_/ http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/browse... File chrome/browser/ui/views/browser_actions_host_delegate.h (right): http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/browse... chrome/browser/ui/views/browser_actions_host_delegate.h:20: class BrowserActionsHostDelegate : public views::DragController { Can this be BrowserActionView::Delegate? That would be the typical convention in Chrome. http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/browse... chrome/browser/ui/views/browser_actions_host_delegate.h:25: virtual void OnBrowserActionVisibilityChanged() = 0; This one is not used. Seems like something only the toolbar would care about, since visibility cannot change for action box. http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/browse... chrome/browser/ui/views/browser_actions_host_delegate.h:26: virtual gfx::Size GetContentOffset() const = 0; This needs a better name. http://codereview.chromium.org/10533086/diff/1/ui/views/controls/menu/menu_it... File ui/views/controls/menu/menu_item_view.h (right): http://codereview.chromium.org/10533086/diff/1/ui/views/controls/menu/menu_it... ui/views/controls/menu/menu_item_view.h:324: virtual MenuItemView* AllocateMenuItemView(MenuItemView* parent, int item_id, You're right, this looks like a funny change. Instead, can we add MenuItemView::AppendItem(MenuItemView*), and then call that instead of AppendItemFromModel()? http://codereview.chromium.org/10533086/diff/1/ui/views/controls/menu/menu_it... ui/views/controls/menu/menu_item_view.h:328: int GetTopMargin(); This isn't called anywhere. Does it need to be public?
http://codereview.chromium.org/10533086/diff/1/chrome/browser/extensions/exte... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/10533086/diff/1/chrome/browser/extensions/exte... chrome/browser/extensions/extension_prefs.cc:72: const char kExtensionActionbox[] = "extensions.actionbox"; On 2012/06/12 05:53:44, Aaron Boodman wrote: > I would recommend: extensions.action_box_order Done. http://codereview.chromium.org/10533086/diff/1/chrome/browser/extensions/exte... chrome/browser/extensions/extension_prefs.cc:74: const char kExtensionActionboxBar[] = "extensions.actionbox_bar"; Changed to toolbar_order. Assuming that old preference eventually will go away, I think toolbar_order would be better. On 2012/06/12 05:53:44, Aaron Boodman wrote: > And: toolbar_order_for_action_box_mode http://codereview.chromium.org/10533086/diff/1/chrome/browser/extensions/exte... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/10533086/diff/1/chrome/browser/extensions/exte... chrome/browser/extensions/extension_prefs.h:112: std::vector<std::string> GetActionboxOrder(); On 2012/06/12 05:53:44, Aaron Boodman wrote: > Nit: I think we should use 'ActionBox', not 'Actionbox'. Done. http://codereview.chromium.org/10533086/diff/1/chrome/browser/extensions/exte... File chrome/browser/extensions/extension_toolbar_model.cc (right): http://codereview.chromium.org/10533086/diff/1/chrome/browser/extensions/exte... chrome/browser/extensions/extension_toolbar_model.cc:204: void ExtensionToolbarModel::InitializeExtensionLists() { On 2012/06/12 05:53:44, Aaron Boodman wrote: > This is pretty hard to follow. How about something like: > > void Populate() { > if (IsActionBoxEnabled()) > PopulateForActionBoxMode(); > else > PopulateForNonActionBoxMode(); > } > > void PopulateForActionBoxMode() { > // all enabled extensions that have browser actions are shown. > // there's no way to hide browser actions in action box mode. > // if an extension is explicitly in the toolbar, put it in > // the toolbar list. otherwise, it goes in the action box list. > } > > void PopulateToolbarForActionBoxMode() { > // old populate logic here. > } > > This would mean undoing some of the visibility changes you did earlier, but it > seems simpler this way. Done. http://codereview.chromium.org/10533086/diff/1/chrome/browser/extensions/exte... File chrome/browser/extensions/extension_toolbar_model.h (right): http://codereview.chromium.org/10533086/diff/1/chrome/browser/extensions/exte... chrome/browser/extensions/extension_toolbar_model.h:99: const extensions::Extension* GetActionBoxExtensionByIndex(int index) const; I'm a bit uncomfortable to do it. I don't want to return a copy of a list, because caller could cache it. Returning a const reference is OK, but I don't see how it is better then current implementation. On 2012/06/12 05:53:44, Aaron Boodman wrote: > Why not just expose the ExtensionList directly? Consider changing the toolbar > one to do the same. http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/toolbar/acti... File chrome/browser/ui/toolbar/action_box_menu_model.h (right): http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/toolbar/acti... chrome/browser/ui/toolbar/action_box_menu_model.h:31: // Overridden for both ButtonMenuItemModel::Delegate and SimpleMenuModel: On 2012/06/12 05:53:44, Aaron Boodman wrote: > If these aren't going to be called except through the interfaces, make them > private. Done. http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/toolbar/acti... chrome/browser/ui/toolbar/action_box_menu_model.h:45: size_t action_box_extensions_size(); Internally in this class everything coming from extensions model. But I can imagine user of ActionBoxMenuModel will need something from extensions too. So these methods provide a way to do it. I understand they not yet used that way, but pretty sure they will be. On 2012/06/12 05:53:44, Aaron Boodman wrote: > This helper doesn't seem necessary. Can just get the value from the model > directly when needed. http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/action... File chrome/browser/ui/views/action_box_menu.cc (right): http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/action... chrome/browser/ui/views/action_box_menu.cc:45: int id = 1; Done Original idea was that it could be called recursively for submenus, but we don't have them right now On 2012/06/12 05:53:44, Aaron Boodman wrote: > This is never used for anything...? Seems like it could be defined inside > PopulateMenu(). http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/action... File chrome/browser/ui/views/action_box_menu.h (right): http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/action... chrome/browser/ui/views/action_box_menu.h:39: void Init(); It is called from ActionBoxButtonView. Potentially it could be removed at all, it is only question how much to put in constructor and how much into this function. Looks like they always will be called one after another. On 2012/06/12 05:53:44, Aaron Boodman wrote: > Make private. http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/action... chrome/browser/ui/views/action_box_menu.h:44: // BrowserActionsHostDelegate overrides: On 2012/06/12 05:53:44, Aaron Boodman wrote: > Make these private. Done. http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/action... File chrome/browser/ui/views/action_box_menu_item_view.cc (right): http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/action... chrome/browser/ui/views/action_box_menu_item_view.cc:14: MenuItemView::Layout(); Not sure it is true, we layout only icon view, and let the base class to do the rest On 2012/06/12 05:53:44, Aaron Boodman wrote: > I think the child layout is unnecessary since we're doing our own custom layout > anyway. http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/action... chrome/browser/ui/views/action_box_menu_item_view.cc:19: gfx::Size size = child->GetPreferredSize(); On 2012/06/12 05:53:44, Aaron Boodman wrote: > child->SetPosition(Position(0, 0)); > child->SizeToPreferredSize(); Done. http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/action... chrome/browser/ui/views/action_box_menu_item_view.cc:33: MenuItemView::OnPaint(canvas); Yes, removed it after it was uploaded On 2012/06/12 05:53:44, Aaron Boodman wrote: > No need to override? http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/browse... File chrome/browser/ui/views/browser_action_view.h (right): http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/browse... chrome/browser/ui/views/browser_action_view.h:117: BrowserActionsHostDelegate* panel_; On 2012/06/12 05:53:44, Aaron Boodman wrote: > s/panel_/delegate_/ Done. http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/browse... File chrome/browser/ui/views/browser_actions_host_delegate.h (right): http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/browse... chrome/browser/ui/views/browser_actions_host_delegate.h:20: class BrowserActionsHostDelegate : public views::DragController { On 2012/06/12 05:53:44, Aaron Boodman wrote: > Can this be BrowserActionView::Delegate? That would be the typical convention in > Chrome. Done. http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/browse... chrome/browser/ui/views/browser_actions_host_delegate.h:25: virtual void OnBrowserActionVisibilityChanged() = 0; this delegate is used by both menu and toolbar On 2012/06/12 05:53:44, Aaron Boodman wrote: > This one is not used. Seems like something only the toolbar would care about, > since visibility cannot change for action box. http://codereview.chromium.org/10533086/diff/1/chrome/browser/ui/views/browse... chrome/browser/ui/views/browser_actions_host_delegate.h:26: virtual gfx::Size GetContentOffset() const = 0; Renamed to GetViewContentOffset() On 2012/06/12 05:53:44, Aaron Boodman wrote: > This needs a better name. http://codereview.chromium.org/10533086/diff/1/ui/views/controls/menu/menu_it... File ui/views/controls/menu/menu_item_view.h (right): http://codereview.chromium.org/10533086/diff/1/ui/views/controls/menu/menu_it... ui/views/controls/menu/menu_item_view.h:324: virtual MenuItemView* AllocateMenuItemView(MenuItemView* parent, int item_id, On 2012/06/12 05:53:44, Aaron Boodman wrote: > You're right, this looks like a funny change. > > Instead, can we add MenuItemView::AppendItem(MenuItemView*), and then call that > instead of AppendItemFromModel()? Done. http://codereview.chromium.org/10533086/diff/1/ui/views/controls/menu/menu_it... ui/views/controls/menu/menu_item_view.h:328: int GetTopMargin(); On 2012/06/12 05:53:44, Aaron Boodman wrote: > This isn't called anywhere. Does it need to be public? Done.
Hi Aaron, could you look at this one I've updated it with latest code
Please make sure to fix the CL description to be more descriptive.
http://codereview.chromium.org/10533086/diff/19001/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.cc:80: const char kExtensionActionBox[] = "extensions.action_box_order"; Comment these. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.cc:82: const char kExtensionActionBoxBar[] = "extensions.toolbar_order"; I still think this should be toolbar_order_with_action_box. It's more characters, but I think the clarity is worth it. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.cc:1927: if (list_of_values) { Reverse check and return early. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.h:546: std::vector<std::string> GetExtensionsOrder(const char* pref); If it's really just getting an array of strings, this can be renamed to be more general, right? http://codereview.chromium.org/10533086/diff/19001/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.cc (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:224: const std::vector<std::string> bar_order = Nit: toolbar_order? http://codereview.chromium.org/10533086/diff/19001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:247: service_->extension_prefs()->GetBrowserActionVisibility(extension); Do we need the pinned preference? Is it enough to just say that a browser action is pinned if its ID shows up in the toolbar order list? Same question for old code ... maybe we can remove that preference? Maybe it was added before these items were re-orderable or something. Adding finnur in case he remembers the history. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:276: sorted->at(index) = extension; Nit: clearer to just say sorted[index] = extension. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.cc (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:23: // ActionBoxMenu --------------------------------------------------------------- Remove this line. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:36: STLDeleteElements(&browser_action_views_); Use std::vector<linked_ptr<BrowserActionView> > to avoid manual delete here. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:40: DCHECK(!root_); CHECK(!root_) http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:42: root_->set_has_icons(true); // We have checks, radios and icons, set this Put this comment on the line above. Multiline trailing comments are hard to maintain. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:42: root_->set_has_icons(true); // We have checks, radios and icons, set this We actually only have icons right, not checks or radios? http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:49: gfx::Point screen_loc; Avoid abbreviation: screen_location http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:53: MenuItemView::TOPRIGHT, views::MenuRunner::HAS_MNEMONICS) == Left column of all params lines should line up. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:64: // views::BubbleBorder* border = new views::BubbleBorder( Kill dead code. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:113: int id = 1; Consider changing this counter into a member variable like next_id_ so that you don't have to pass it around. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:148: MenuItemView::NORMAL); Don't any of the AppendMenuItem* methods work? If so, you could get rid of the |index| variable from here and the call site. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.h (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:43: // MenuDelegate overrides: Indent is incorrect. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:44: virtual void ExecuteCommand(int id); Why is OVERRIDE not needed here? http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:47: Remove extra blank line. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:56: // Overridden from views::DragController: Inconsistent comment wording: views::DragController overrides? http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:59: ui::OSExchangeData* data) OVERRIDE; The left column of all param lines should line up. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu_item_view.cc (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu_item_view.cc:10: // ActionBoxMenuItemView ------------------------------------------------------- Delete this line. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu_item_view.cc:45: case ui::MenuModel::TYPE_COMMAND: We don't expect to be receiving most of these types, right? And they probably won't layout/paint nicely in any case? If, so don't handle them, and let fall through to NOTREACHED(). http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu_item_view.h (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu_item_view.h:13: // ActionBoxMenuItemView ------------------------------------------------------- Remove this line. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu_item_view.h:17: Remove this line. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu_item_view.h:21: virtual void OnPaint(gfx::Canvas* canvas) OVERRIDE; Does this need to be public? http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu_item_view.h:24: virtual void Layout() OVERRIDE; Does this need to be public? http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.cc (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:166: SetTooltipText(name); !? http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:304: void BrowserActionButton::DisableTooltip(bool disable_tooltip) { If this method is going to accept a value, it should be called SetTooltipIsEnabled() http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:307: SetTooltipText(string16()); Don't you need to reset it to the string in the case where it's enabled? http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:391: gfx::Size size = delegate_->GetViewContentOffset(); offset? http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:403: return gfx::Size(Extension::kBrowserActionIconMaxSize+10, // FIXME Fix before checking in. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.h (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:30: Delete this line. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:39: virtual gfx::Size GetViewContentOffset() const = 0; This name is not very descriptive. Can you pick a better one? Also all of these need comments. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:83: public views::ButtonListener, These should line up as they did before. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:88: BrowserActionView::Delegate* delegate); Put indent back as before. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:111: const views::Event& event) OVERRIDE; Why are you changing all these indents? http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:144: void DisableTooltip(bool disable_tooltip); Hm, we should do something with the tooltip text though. Perhaps we can put it underneath as smaller, lighter text. Can you open a bug for this and assign to roma? http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:189: bool disable_tooltip_; Comment. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_actions_container.h (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_actions_container.h:160: virtual gfx::Size GetViewContentOffset() const OVERRIDE; Put these into a // Overrridden from BrowserActionView::Delegate section and make them private. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.cc (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.cc:79: bookmark_state_ = on; Do you need to trigger a paint here? http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.cc:97: Browser* browser = Add blank line before this one. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.cc:99: Remove blank line. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.h (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.h:39: LocationBarView::Delegate* delegate_; Weird to store another class' delegate. Seems like this is only being used to get the Browser. Can you have LocationView pass in the browser instead? LocationBar already has GetBrowserFromDelegate() that you can call. http://codereview.chromium.org/10533086/diff/19001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view.h (right): http://codereview.chromium.org/10533086/diff/19001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:139: Remove blank line.
You should also ask sky to review this (after we're done) since he reviewed the setup for this.
http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.cc (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:20: using views::MenuItemView; nit: please, avoid using declarations if possible. Up to you though! http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:36: STLDeleteElements(&browser_action_views_); On 2012/07/02 22:41:34, Aaron Boodman wrote: > Use std::vector<linked_ptr<BrowserActionView> > to avoid manual delete here. Ham? What is that? :/ Do you mean using ScopedVector instead?
http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.cc (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:36: STLDeleteElements(&browser_action_views_); On 2012/07/02 22:49:18, tfarina wrote: > On 2012/07/02 22:41:34, Aaron Boodman wrote: > > Use std::vector<linked_ptr<BrowserActionView> > to avoid manual delete here. > Ham? What is that? :/ > > Do you mean using ScopedVector instead? What is what? base/memory/linked_ptr.h
On Mon, Jul 2, 2012 at 9:03 PM, <aa@chromium.org> wrote: > > http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... > File chrome/browser/ui/views/action_box_menu.cc (right): > > http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... > chrome/browser/ui/views/action_box_menu.cc:36: > STLDeleteElements(&browser_action_views_); > On 2012/07/02 22:49:18, tfarina wrote: >> >> On 2012/07/02 22:41:34, Aaron Boodman wrote: >> > Use std::vector<linked_ptr<BrowserActionView> > to avoid manual > > delete here. >> >> Ham? What is that? :/ > > >> Do you mean using ScopedVector instead? > > > What is what? base/memory/linked_ptr.h > I know linked_ptr.h is in base/memory, just the suggestion appeared to be odd to me, because I haven't see it before. And my first thought was, hey this looks funny, why not ScopedVector, which seems more natural to me. Though a grep reveals it's heavily used in chrome/browser/extensions/api/, but no anywhere in chrome/browser/ui/views/ -- Thiago
$ git grep linked_ptr | wc -l 749 $ git grep linked_ptr | grep -v extensions | wc -l 439 $ git grep ScopedVector | wc -l 357
http://codereview.chromium.org/10533086/diff/19001/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.cc:80: const char kExtensionActionBox[] = "extensions.action_box_order"; On 2012/07/02 22:41:34, Aaron Boodman wrote: > Comment these. Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.cc:82: const char kExtensionActionBoxBar[] = "extensions.toolbar_order"; On 2012/07/02 22:41:34, Aaron Boodman wrote: > I still think this should be toolbar_order_with_action_box. It's more > characters, but I think the clarity is worth it. Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.cc:1927: if (list_of_values) { On 2012/07/02 22:41:34, Aaron Boodman wrote: > Reverse check and return early. Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.h:546: std::vector<std::string> GetExtensionsOrder(const char* pref); On 2012/07/02 22:41:34, Aaron Boodman wrote: > If it's really just getting an array of strings, this can be renamed to be more > general, right? Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.cc (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:224: const std::vector<std::string> bar_order = On 2012/07/02 22:41:34, Aaron Boodman wrote: > Nit: toolbar_order? Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:247: service_->extension_prefs()->GetBrowserActionVisibility(extension); yes, I think pinned pref could be removed. I've changed code here and will remove pinned in a separate CL On 2012/07/02 22:41:34, Aaron Boodman wrote: > Do we need the pinned preference? Is it enough to just say that a browser action > is pinned if its ID shows up in the toolbar order list? > > Same question for old code ... maybe we can remove that preference? Maybe it was > added before these items were re-orderable or something. > > Adding finnur in case he remembers the history. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:276: sorted->at(index) = extension; sorted is a pointer so it would be (*sorted)[index] = ... I think sorted->at(index) = ... is more readable On 2012/07/02 22:41:34, Aaron Boodman wrote: > Nit: clearer to just say sorted[index] = extension. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.cc (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:20: using views::MenuItemView; On 2012/07/02 22:49:18, tfarina wrote: > nit: please, avoid using declarations if possible. Up to you though! Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:23: // ActionBoxMenu --------------------------------------------------------------- On 2012/07/02 22:41:34, Aaron Boodman wrote: > Remove this line. Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:36: STLDeleteElements(&browser_action_views_); On 2012/07/02 22:41:34, Aaron Boodman wrote: > Use std::vector<linked_ptr<BrowserActionView> > to avoid manual delete here. Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:40: DCHECK(!root_); On 2012/07/02 22:41:34, Aaron Boodman wrote: > CHECK(!root_) Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:42: root_->set_has_icons(true); // We have checks, radios and icons, set this On 2012/07/02 22:41:34, Aaron Boodman wrote: > Put this comment on the line above. Multiline trailing comments are hard to > maintain. Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:49: gfx::Point screen_loc; On 2012/07/02 22:41:34, Aaron Boodman wrote: > Avoid abbreviation: screen_location Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:53: MenuItemView::TOPRIGHT, views::MenuRunner::HAS_MNEMONICS) == On 2012/07/02 22:41:34, Aaron Boodman wrote: > Left column of all params lines should line up. Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:64: // views::BubbleBorder* border = new views::BubbleBorder( On 2012/07/02 22:41:34, Aaron Boodman wrote: > Kill dead code. Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:113: int id = 1; Would be weird to have a member variable that keeps state after function call, when this function is called only one time and variable is used inside that function only. On 2012/07/02 22:41:34, Aaron Boodman wrote: > Consider changing this counter into a member variable like next_id_ so that you > don't have to pass it around. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:148: MenuItemView::NORMAL); On 2012/07/02 22:41:34, Aaron Boodman wrote: > Don't any of the AppendMenuItem* methods work? If so, you could get rid of the > |index| variable from here and the call site. Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.h (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:43: // MenuDelegate overrides: On 2012/07/02 22:41:34, Aaron Boodman wrote: > Indent is incorrect. Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:44: virtual void ExecuteCommand(int id); On 2012/07/02 22:41:34, Aaron Boodman wrote: > Why is OVERRIDE not needed here? Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:47: On 2012/07/02 22:41:34, Aaron Boodman wrote: > Remove extra blank line. Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:56: // Overridden from views::DragController: On 2012/07/02 22:41:34, Aaron Boodman wrote: > Inconsistent comment wording: views::DragController overrides? Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:59: ui::OSExchangeData* data) OVERRIDE; On 2012/07/02 22:41:34, Aaron Boodman wrote: > The left column of all param lines should line up. Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.cc (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:166: SetTooltipText(name); On 2012/07/02 22:41:34, Aaron Boodman wrote: > !? Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:304: void BrowserActionButton::DisableTooltip(bool disable_tooltip) { Renamed to SetTooltipDisabled(...) On 2012/07/02 22:41:34, Aaron Boodman wrote: > If this method is going to accept a value, it should be called > SetTooltipIsEnabled() http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:307: SetTooltipText(string16()); On 2012/07/02 22:41:34, Aaron Boodman wrote: > Don't you need to reset it to the string in the case where it's enabled? Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:391: gfx::Size size = delegate_->GetViewContentOffset(); I know it is coming back over and over:) Button inside the view could be offset by x, y. I mean left upper corner of the button has position x,y I call it offset. On 2012/07/02 22:41:34, Aaron Boodman wrote: > offset? http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:403: return gfx::Size(Extension::kBrowserActionIconMaxSize+10, // FIXME On 2012/07/02 22:41:34, Aaron Boodman wrote: > Fix before checking in. Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.h (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:30: On 2012/07/02 22:41:34, Aaron Boodman wrote: > Delete this line. Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:39: virtual gfx::Size GetViewContentOffset() const = 0; I have a hard time to come up with a better name, added comment. On 2012/07/02 22:41:34, Aaron Boodman wrote: > This name is not very descriptive. Can you pick a better one? Also all of these > need comments. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:83: public views::ButtonListener, Done I have a devstudio plugin which tries to do smart formatting on copy/paste, I usually fix it after, missed it here On 2012/07/02 22:41:34, Aaron Boodman wrote: > These should line up as they did before. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:88: BrowserActionView::Delegate* delegate); On 2012/07/02 22:41:34, Aaron Boodman wrote: > Put indent back as before. Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:111: const views::Event& event) OVERRIDE; On 2012/07/02 22:41:34, Aaron Boodman wrote: > Why are you changing all these indents? Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:144: void DisableTooltip(bool disable_tooltip); This one disables tooltip for an browser action button, the one we need should be for menu item. ok (for bug) On 2012/07/02 22:41:34, Aaron Boodman wrote: > Hm, we should do something with the tooltip text though. Perhaps we can put it > underneath as smaller, lighter text. > > Can you open a bug for this and assign to roma? http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:189: bool disable_tooltip_; On 2012/07/02 22:41:34, Aaron Boodman wrote: > Comment. Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_actions_container.h (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_actions_container.h:160: virtual gfx::Size GetViewContentOffset() const OVERRIDE; At least one function from BrowserActionView::Delegate is used in BrowserActionOverflowMenuController and need to be public, I kept all of them public, only grouped together. On 2012/07/02 22:41:34, Aaron Boodman wrote: > Put these into a // Overrridden from BrowserActionView::Delegate section and > make them private. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.cc (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.cc:79: bookmark_state_ = on; I don't think so, button by itself doesn't show bookmark icon, it will only pass this value to the menu when pressed. On 2012/07/02 22:41:34, Aaron Boodman wrote: > Do you need to trigger a paint here? http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.cc:97: Browser* browser = On 2012/07/02 22:41:34, Aaron Boodman wrote: > Add blank line before this one. Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.cc:99: On 2012/07/02 22:41:34, Aaron Boodman wrote: > Remove blank line. Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.h (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.h:39: LocationBarView::Delegate* delegate_; The problem is at the time of button creation GetBrowserFromDelegate() returns NULL, because WebContents dont exist yet. So I need some way to call back to LocationBarView at the time when button is pressed. On 2012/07/02 22:41:34, Aaron Boodman wrote: > Weird to store another class' delegate. Seems like this is only being used to > get the Browser. Can you have LocationView pass in the browser instead? > LocationBar already has GetBrowserFromDelegate() that you can call. http://codereview.chromium.org/10533086/diff/19001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_item_view.h (right): http://codereview.chromium.org/10533086/diff/19001/ui/views/controls/menu/men... ui/views/controls/menu/menu_item_view.h:139: It was a merging issue, it should not be here at all, removed On 2012/07/02 22:41:34, Aaron Boodman wrote: > Remove blank line.
http://codereview.chromium.org/10533086/diff/19001/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.cc (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:276: sorted->at(index) = extension; On 2012/07/11 22:34:34, yefimt wrote: > sorted is a pointer so it would be (*sorted)[index] = ... > I think sorted->at(index) = ... is more readable > No, don't use std::vector<T>::at. See Peter's recommendation at http://codereview.chromium.org/10458045/
almost there! http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.h (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.h:39: LocationBarView::Delegate* delegate_; In that case, define a delegate for ActionBoxButtonView that only has GetBrowser(), and have LocationBar implement it. It's more typing, but better layering/encapsulation. On 2012/07/11 22:34:34, yefimt wrote: > The problem is at the time of button creation GetBrowserFromDelegate() returns > NULL, because WebContents dont exist yet. So I need some way to call back to > LocationBarView at the time when button is pressed. > > On 2012/07/02 22:41:34, Aaron Boodman wrote: > > Weird to store another class' delegate. Seems like this is only being used to > > get the Browser. Can you have LocationView pass in the browser instead? > > LocationBar already has GetBrowserFromDelegate() that you can call. >
http://codereview.chromium.org/10533086/diff/19001/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.cc (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:276: sorted->at(index) = extension; On 2012/07/12 01:21:43, tfarina wrote: > On 2012/07/11 22:34:34, yefimt wrote: > > sorted is a pointer so it would be (*sorted)[index] = ... > > I think sorted->at(index) = ... is more readable > > > No, don't use std::vector<T>::at. > > See Peter's recommendation at http://codereview.chromium.org/10458045/ Done. http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.h (right): http://codereview.chromium.org/10533086/diff/19001/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.h:39: LocationBarView::Delegate* delegate_; On 2012/07/13 01:53:07, Aaron Boodman wrote: > In that case, define a delegate for ActionBoxButtonView that only has > GetBrowser(), and have LocationBar implement it. > > It's more typing, but better layering/encapsulation. > > On 2012/07/11 22:34:34, yefimt wrote: > > The problem is at the time of button creation GetBrowserFromDelegate() returns > > NULL, because WebContents dont exist yet. So I need some way to call back to > > LocationBarView at the time when button is pressed. > > > > On 2012/07/02 22:41:34, Aaron Boodman wrote: > > > Weird to store another class' delegate. Seems like this is only being used > to > > > get the Browser. Can you have LocationView pass in the browser instead? > > > LocationBar already has GetBrowserFromDelegate() that you can call. > > > Done.
LGTM!
adding owners pkasting@ for chrome/browser/ui and jhawkins@ for chrome chrome_browser.gypi
Rubber-stamp LGTM
http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/action_box_menu_model.cc (right): http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.cc:7: #include <algorithm> Nit: Why are either of these #includes necessary? http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.cc:44: const content::NotificationSource& source, Nit: Incorrect indenting http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.cc:49: ExtensionToolbarModel* toolbar_model = extension_service_->toolbar_model(); Nit: Inline into next line (2 places) http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.cc:54: ActionBoxMenuModel::GetActionBoxExtensionByIndex(int index) { Nit: Indent 4 http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.cc:60: int command_id = 1000; Nit: What is this magic number? http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.cc:61: size_t size = action_box_extensions_size(); Nit: Inline into loop declaration http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.cc:64: string16 label = UTF8ToUTF16(extension->name()); Nit: Inline into next line http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/action_box_menu_model.h (right): http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:9: #include "base/file_path.h" Nit: Add blank line here http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:40: int command_id, ui::Accelerator* accelerator) OVERRIDE; Nit: One arg per line http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:47: void Build(); What does this do? http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:51: typedef std::map<int, std::string> IdToEntensionId; Nit: typedef goes atop the private section. Put "Map" in the name. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.cc (right): http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:18: // ActionBoxMenu --------------------------------------------------------------- In some files you use dividers like: /////////... // Foo And in some you use this style. Be consistent across all files (and ideally match the rest of the location bar code). Similarly, be consistent across all files about whether there is one or two blank lines between #includes and everything else. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:19: ActionBoxMenu::ActionBoxMenu(Browser* browser, Nit: Blank line here http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:43: gfx::Rect bounds(screen_location, host->size()); Nit: Inline into next statement http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:44: if (menu_runner_->RunMenuAt(host->GetWidget(), You're not doing anything special for MENU_DELETED, so don't check the return value. Use ignore_result() instead and add a comment (see e.g. infobar_view.cc). http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:58: return views::Border::CreateSolidBorder(1, SkColorSetRGB(0, 0, 0)); Don't hardcode colors. You need to use the appropriate system/theme colors. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:105: int item_id = 1; Nit: If you name this |bookmark_item_id|, you can replace the use in the loop below with "bookmark_item_id + model_index" and eliminate the increment. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:129: views::MenuItemView* parent, int* item_id) { Nit: One arg per line http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:130: string16 label = Nit: Wrap like: string16 label = l10n_util::GetStringUTF16(bookmark_item_state_ ? IDS_TOOLTIP_STARRED : IDS_TOOLTIP_STAR); http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.h (right): http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:29: : public views::MenuDelegate, Nit: Place at end of previous line, indent subsequent lines to match http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:33: ActionBoxMenu(Browser* browser, ActionBoxMenuModel* model, Nit: One arg per line http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:39: // Shows the menu relative to the specified view. Nit: "specified button" perhaps? http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:40: void RunMenu(views::MenuButton* host); Nit: |host| seems like an odd name, what about |button|? http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:88: typedef std::vector<linked_ptr<BrowserActionView> > BrowserActionViews; Use ScopedVector instead. Nit: Typedef goes atop the private section. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.cc (right): http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:160: string16 name = GetTextForTooltip(); Nit: Inline in next statement (both shorter and more efficient) http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:355: // BrowserActionView Nit: Class definition order must match declaration order (BrowserActionView was declared before BrowserActionButton) http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.h (right): http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:31: class Delegate : public views::DragController { Why is the delegate a DragController? Either the delegate implementations should directly inherit this themselves or you should comment here about why this is and what functions you need. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:34: virtual Browser* GetBrowser() const = 0; Why add this getter instead of just passing the Browser* in? http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:45: // Returns offset of a content inside the view. Nit: "a content" is not grammatical, and regardless, I don't really understand what this is saying. What content in what view? http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:66: Nit: Remove blank line http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:156: View* parent, Nit: Why did you change the previous correct indentation to this incorrect indentation? http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_actions_container.cc (right): http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_actions_container.cc:139: BrowserActionView* BrowserActionsContainer::GetBrowserActionView( Nit: Declaration and definition orders should match. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.cc (right): http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.cc:93: Browser* browser = delegate_->GetBrowser(); Why did you choose to make this a delegate method instead of passing the Browser* in to the constructor? Seems like the latter would allow you to omit the delegate class entirely. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.cc:101: action_box_menu_.reset(NULL); Nit: Omit NULL http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.h (right): http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.h:24: Delegate() {} Nit: No need to supply the constructor. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.h:35: void SetBookmarkState(bool on); Nit: Should be inlined and named set_bookmark_state(bool bookmark_state_) http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.h:46: Nit: Remove blank lines
http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/action_box_menu_model.cc (right): http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.cc:7: #include <algorithm> On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: Why are either of these #includes necessary? Done. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.cc:44: const content::NotificationSource& source, On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: Incorrect indenting Done. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.cc:49: ExtensionToolbarModel* toolbar_model = extension_service_->toolbar_model(); On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: Inline into next line (2 places) Done. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.cc:54: ActionBoxMenuModel::GetActionBoxExtensionByIndex(int index) { On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: Indent 4 Done. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.cc:60: int command_id = 1000; On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: What is this magic number? Done. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.cc:61: size_t size = action_box_extensions_size(); On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: Inline into loop declaration Done. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.cc:64: string16 label = UTF8ToUTF16(extension->name()); On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: Inline into next line Done. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/action_box_menu_model.h (right): http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:9: #include "base/file_path.h" On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: Add blank line here Done. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:40: int command_id, ui::Accelerator* accelerator) OVERRIDE; On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: One arg per line Done. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:47: void Build(); On 2012/07/14 02:08:01, Peter Kasting wrote: > What does this do? Done. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:51: typedef std::map<int, std::string> IdToEntensionId; On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: typedef goes atop the private section. Put "Map" in the name. Done. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.cc (right): http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:18: // ActionBoxMenu --------------------------------------------------------------- On 2012/07/14 02:08:01, Peter Kasting wrote: > In some files you use dividers like: > > /////////... > // Foo > > And in some you use this style. Be consistent across all files (and ideally > match the rest of the location bar code). > > Similarly, be consistent across all files about whether there is one or two > blank lines between #includes and everything else. Done. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:19: ActionBoxMenu::ActionBoxMenu(Browser* browser, On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: Blank line here Done. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:43: gfx::Rect bounds(screen_location, host->size()); On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: Inline into next statement Done. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:44: if (menu_runner_->RunMenuAt(host->GetWidget(), On 2012/07/14 02:08:01, Peter Kasting wrote: > You're not doing anything special for MENU_DELETED, so don't check the return > value. Use ignore_result() instead and add a comment (see e.g. > infobar_view.cc). Done. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:58: return views::Border::CreateSolidBorder(1, SkColorSetRGB(0, 0, 0)); I don't have written specs yet, just mocks, but I'm pretty sure they want specific colors, not theme colors. On 2012/07/14 02:08:01, Peter Kasting wrote: > Don't hardcode colors. You need to use the appropriate system/theme colors. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:105: int item_id = 1; It is going to be a couple more menu items soon (before extensions). On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: If you name this |bookmark_item_id|, you can replace the use in the loop > below with "bookmark_item_id + model_index" and eliminate the increment. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:129: views::MenuItemView* parent, int* item_id) { On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: One arg per line Done. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:130: string16 label = On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: Wrap like: > > string16 label = l10n_util::GetStringUTF16(bookmark_item_state_ ? > IDS_TOOLTIP_STARRED : IDS_TOOLTIP_STAR); Done. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.h (right): http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:29: : public views::MenuDelegate, On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: Place at end of previous line, indent subsequent lines to match Done. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:33: ActionBoxMenu(Browser* browser, ActionBoxMenuModel* model, On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: One arg per line Done. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:39: // Shows the menu relative to the specified view. On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: "specified button" perhaps? Done. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:40: void RunMenu(views::MenuButton* host); On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: |host| seems like an odd name, what about |button|? Done. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:88: typedef std::vector<linked_ptr<BrowserActionView> > BrowserActionViews; Actually another reviewer, aa@, insisted to use vector<linked_ptr<...> > instead of ScopedVector On 2012/07/14 02:08:01, Peter Kasting wrote: > Use ScopedVector instead. > > Nit: Typedef goes atop the private section. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.cc (right): http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:160: string16 name = GetTextForTooltip(); It is used in the next two lines, would it be better to get it one time, instead of two? On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: Inline in next statement (both shorter and more efficient) http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:355: // BrowserActionView Done I thought it will be easier to review if I keep original order On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: Class definition order must match declaration order (BrowserActionView was > declared before BrowserActionButton) http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.h (right): http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:31: class Delegate : public views::DragController { They could implement it by themself, but then BrowserActionButton will not be able to call Drag* functions. On 2012/07/14 02:08:01, Peter Kasting wrote: > Why is the delegate a DragController? Either the delegate implementations > should directly inherit this themselves or you should comment here about why > this is and what functions you need. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:34: virtual Browser* GetBrowser() const = 0; It is possible, I just didnt want to change existing implementation. Old code used to call BrowserActionsContainer to get browser, now it does the same to whoever implements delagate On 2012/07/14 02:08:01, Peter Kasting wrote: > Why add this getter instead of just passing the Browser* in? http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:45: // Returns offset of a content inside the view. Ok, I got a few comments about it, I guess I cannot find a better name. This class is a view, and it has some contents (a button). This function supposed to return offset (position) of contents (button) relative to upper left corner. On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: "a content" is not grammatical, and regardless, I don't really understand > what this is saying. What content in what view? http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:66: On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: Remove blank line Done. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:156: View* parent, It is not exactly me, Visual Assist I have in devstudio re-formatted it when I copy pasted that code, I fixed what I noticed but missed a few. Fixed. On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: Why did you change the previous correct indentation to this incorrect > indentation? http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_actions_container.cc (right): http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_actions_container.cc:139: BrowserActionView* BrowserActionsContainer::GetBrowserActionView( On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: Declaration and definition orders should match. Done. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.cc (right): http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.cc:93: Browser* browser = delegate_->GetBrowser(); First of all it was done this way before my change. BrowserActionView (and BrowserActionButton) were used inside BrowserActionContainer, and were calling a few functions from it, including GetBrowser. My change introduced menu that can host BrowserActionView and BrowserActionButton, so I just moved functions they call to a delegate. On 2012/07/14 02:08:01, Peter Kasting wrote: > Why did you choose to make this a delegate method instead of passing the > Browser* in to the constructor? Seems like the latter would allow you to omit > the delegate class entirely. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.cc:101: action_box_menu_.reset(NULL); On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: Omit NULL Done. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.h (right): http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.h:24: Delegate() {} On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: No need to supply the constructor. Done. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.h:35: void SetBookmarkState(bool on); On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: Should be inlined and named set_bookmark_state(bool bookmark_state_) Done. http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.h:46: On 2012/07/14 02:08:01, Peter Kasting wrote: > Nit: Remove blank lines Done.
Will re-review later. On 2012/07/17 18:20:37, yefimt wrote: > http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... > chrome/browser/ui/views/action_box_menu.cc:58: return > views::Border::CreateSolidBorder(1, SkColorSetRGB(0, 0, 0)); > I don't have written specs yet, just mocks, but I'm pretty sure they want > specific colors, not theme colors. All browser UI must be compatible with arbitrary system themes and Chrome themes. See for example how we do the existing star bubble or omnibox dropdown for examples of how this is done. You'll generally only get mocks for some typical set of system colors and you have to work out how to handle the other cases. Luckily there are already helper functions for you to get various theme-appropriate colors as well as to help make your own in cases where you need to do something custom. > http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... > chrome/browser/ui/views/action_box_menu.h:88: typedef > std::vector<linked_ptr<BrowserActionView> > BrowserActionViews; > Actually another reviewer, aa@, insisted to use vector<linked_ptr<...> > instead > of ScopedVector Use ScopedVector in Chrome code. I sent an email about this to chromium-dev. > http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/br... > chrome/browser/ui/views/browser_action_view.cc:160: string16 name = > GetTextForTooltip(); > It is used in the next two lines, would it be better to get it one time, instead > of two? Oops, missed that. Ignore. > http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/br... > chrome/browser/ui/views/browser_action_view.h:31: class Delegate : public > views::DragController { > They could implement it by themself, but then BrowserActionButton will not be > able to call Drag* functions. If it needs to do that, then explicitly say what it needs at the time you make this declaration so it's clear why you're making it and that it's not a random orphaned bit. > http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/br... > chrome/browser/ui/views/browser_action_view.h:45: // Returns offset of a content > inside the view. > Ok, I got a few comments about it, I guess I cannot find a better name. > This class is a view, and it has some contents (a button). This function > supposed to return offset (position) of contents (button) relative to upper left > corner. The issue was that the comment needed to explicitly refer to member object names or something instead of just vaguely talking about content. > http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/lo... > chrome/browser/ui/views/location_bar/action_box_button_view.cc:93: Browser* > browser = delegate_->GetBrowser(); > First of all it was done this way before my change. > BrowserActionView (and BrowserActionButton) were used inside > BrowserActionContainer, and were calling a few functions from it, including > GetBrowser. > My change introduced menu that can host BrowserActionView and > BrowserActionButton, so I just moved functions they call to a delegate. If you're touching code, rewrite it in the smartest way, rather than seeking to preserve the original as much as possible. (Applies in both places where you said something similar.)
Here is chrome when Windows high contrast black scheme is selected, star bubble preserved background and border colors. And it make sense because it is displayed within chrome boundaries, which have same colors [image: Inline image 1] On Tue, Jul 17, 2012 at 11:44 AM, <pkasting@chromium.org> wrote: > Will re-review later. > > > On 2012/07/17 18:20:37, yefimt wrote: > > http://codereview.chromium.**org/10533086/diff/18015/** > chrome/browser/ui/views/**action_box_menu.cc#newcode58<http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/action_box_menu.cc#newcode58> > >> chrome/browser/ui/views/**action_box_menu.cc:58: return >> views::Border::**CreateSolidBorder(1, SkColorSetRGB(0, 0, 0)); >> I don't have written specs yet, just mocks, but I'm pretty sure they want >> specific colors, not theme colors. >> > > All browser UI must be compatible with arbitrary system themes and Chrome > themes. See for example how we do the existing star bubble or omnibox > dropdown > for examples of how this is done. You'll generally only get mocks for some > typical set of system colors and you have to work out how to handle the > other > cases. Luckily there are already helper functions for you to get various > theme-appropriate colors as well as to help make your own in cases where > you > need to do something custom. > > > > http://codereview.chromium.**org/10533086/diff/18015/** > chrome/browser/ui/views/**action_box_menu.h#newcode88<http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/action_box_menu.h#newcode88> > >> chrome/browser/ui/views/**action_box_menu.h:88: typedef >> std::vector<linked_ptr<**BrowserActionView> > BrowserActionViews; >> Actually another reviewer, aa@, insisted to use vector<linked_ptr<...> > >> > instead > >> of ScopedVector >> > > Use ScopedVector in Chrome code. I sent an email about this to > chromium-dev. > > > > http://codereview.chromium.**org/10533086/diff/18015/** > chrome/browser/ui/views/**browser_action_view.cc#**newcode160<http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/browser_action_view.cc#newcode160> > >> chrome/browser/ui/views/**browser_action_view.cc:160: string16 name = >> GetTextForTooltip(); >> It is used in the next two lines, would it be better to get it one time, >> > instead > >> of two? >> > > Oops, missed that. Ignore. > > > > http://codereview.chromium.**org/10533086/diff/18015/** > chrome/browser/ui/views/**browser_action_view.h#**newcode31<http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/browser_action_view.h#newcode31> > >> chrome/browser/ui/views/**browser_action_view.h:31: class Delegate : >> public >> views::DragController { >> They could implement it by themself, but then BrowserActionButton will >> not be >> able to call Drag* functions. >> > > If it needs to do that, then explicitly say what it needs at the time you > make > this declaration so it's clear why you're making it and that it's not a > random > orphaned bit. > > > > http://codereview.chromium.**org/10533086/diff/18015/** > chrome/browser/ui/views/**browser_action_view.h#**newcode45<http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/browser_action_view.h#newcode45> > >> chrome/browser/ui/views/**browser_action_view.h:45: // Returns offset of >> a >> > content > >> inside the view. >> Ok, I got a few comments about it, I guess I cannot find a better name. >> This class is a view, and it has some contents (a button). This function >> supposed to return offset (position) of contents (button) relative to >> upper >> > left > >> corner. >> > > The issue was that the comment needed to explicitly refer to member object > names > or something instead of just vaguely talking about content. > > > > http://codereview.chromium.**org/10533086/diff/18015/** > chrome/browser/ui/views/**location_bar/action_box_** > button_view.cc#newcode93<http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/location_bar/action_box_button_view.cc#newcode93> > >> chrome/browser/ui/views/**location_bar/action_box_**button_view.cc:93: >> Browser* >> browser = delegate_->GetBrowser(); >> First of all it was done this way before my change. >> BrowserActionView (and BrowserActionButton) were used inside >> BrowserActionContainer, and were calling a few functions from it, >> including >> GetBrowser. >> My change introduced menu that can host BrowserActionView and >> BrowserActionButton, so I just moved functions they call to a delegate. >> > > If you're touching code, rewrite it in the smartest way, rather than > seeking to > preserve the original as much as possible. (Applies in both places where > you > said something similar.) > > http://codereview.chromium.**org/10533086/<http://codereview.chromium.org/105... >
On Tue, Jul 17, 2012 at 3:46 PM, Yefim Tetelman <yefim@google.com> wrote: > Here is chrome when Windows high contrast black scheme is selected, star > bubble preserved background and border colors. Make sure you restart Chrome after changing themes before testing. If after restarting the star bubble still looks like this, it's a bug. The bubble background should be black in this case (and certainly used to). PK
Ok, it is black after restart On Tue, Jul 17, 2012 at 4:01 PM, Peter Kasting <pkasting@chromium.org>wrote: > On Tue, Jul 17, 2012 at 3:46 PM, Yefim Tetelman <yefim@google.com> wrote: > >> Here is chrome when Windows high contrast black scheme is selected, star >> bubble preserved background and border colors. > > > Make sure you restart Chrome after changing themes before testing. > > If after restarting the star bubble still looks like this, it's a bug. > The bubble background should be black in this case (and certainly used to). > > PK >
It actually brings more generic question. When I look at NativeThemeWin::PaintMenuItemBackground() it uses different color, not the one UI designers want. I feel it should be more centralized effort to implement new UI for all controls (menu, buttons, checkboxes and so on) instead me introducing PaintMenuItemBackground2() or something like that. On Tue, Jul 17, 2012 at 4:05 PM, Yefim Tetelman <yefim@google.com> wrote: > Ok, it is black after restart > > > On Tue, Jul 17, 2012 at 4:01 PM, Peter Kasting <pkasting@chromium.org>wrote: > >> On Tue, Jul 17, 2012 at 3:46 PM, Yefim Tetelman <yefim@google.com> wrote: >> >>> Here is chrome when Windows high contrast black scheme is selected, star >>> bubble preserved background and border colors. >> >> >> Make sure you restart Chrome after changing themes before testing. >> >> If after restarting the star bubble still looks like this, it's a bug. >> The bubble background should be black in this case (and certainly used to). >> >> PK >> > >
On Tue, Jul 17, 2012 at 5:24 PM, Yefim Tetelman <yefim@google.com> wrote: > It actually brings more generic question. > > When I look at NativeThemeWin::PaintMenuItemBackground() it uses different > color, not the one UI designers want. > I feel it should be more centralized effort to implement new UI for all > controls (menu, buttons, checkboxes and so on) > instead me introducing PaintMenuItemBackground2() or something like that. > Don't introduce PaintMenuItemBackground2(), and don't get sidetracked trying to implement new UI for the entirety of Chrome. Use the existing mechanisms and colors and tackle any problems therein as separate bugs. Remember that mocks from the UI designers are just mocks. They are not concrete rules. It is up to us as engineers to implement the right thing ultimately. PK
Here's the rest of my review feedback, besides my earlier replies that still stand. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.cc:1189: return GetExtensionPrefAsVector(action_box_enabled ? kExtensionActionBoxBar : Nit: Break after '?' (or just after '('), not ':' (2 places) http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.cc:1974: iter != strings.end(); ++iter) { Nit: No {} http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.h:551: // Helper function to Get/Set array of strings from/to prefs Nit: Trailing period http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.h:553: void SetExtensionPrefFromVector(const char* pref, Nit: Args must be aligned http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.cc (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:224: void ExtensionToolbarModel::PopulateForActionBoxMode() { Nit: Please put at least your new functions in the same definition order as declaration order. (LGTM in advance if anything else in this file is out of order and you want to TBR those changes separately ahead of time.) http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:225: const std::vector<std::string> toolbar_order = Nit: There's a lot of usage of vector<string> in the header and implementation, you should probably typedef it somewhere. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:231: for (ExtensionSet::const_iterator it = service_->extensions()->begin(); Tiny nit: Be consistent across the files in your change about whether you name iterators |i|, |it|, or |iter|. (Personally I prefer |i| but it doesn't really matter much.) http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:243: (action_box_pos == action_box_order.end())) { Nit: No {} http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:267: int index = std::distance(order.begin(), pos); Nit: Just use "pos - order.begin()"; inline into next statement http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:280: ++iter) { Nit: Why is this on a separate line? http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:288: const std::vector<std::string>& order, ExtensionList* result_list) { Nit: One arg per line http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:340: ids.clear(); Nit: Use a new temp instead of clearing the old one. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:378: return action_box_menu_items_[index]; Nit: Why not inline this as a unix_hacker()-style function like you did with the size accessor? (The same could be done with GetExtensionByIndex()) http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/action_box_menu_model.cc (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.cc:11: static const int kFirstExtensionCommandId = 1000; Nit: Comment on whether and how this interacts with other command ID ranges reserved in various headers; it's not obvious as a reader that using the values around 1000 is inherently safe. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/action_box_menu_model.h (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:38: // Overridden for both ButtonMenuItemModel::Delegate and SimpleMenuModel. Nit: This class does not subclass ButtonMenuItemModel::Delegate, what is this part of the comment about? http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.cc (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:34: CHECK(!root_); Nit: Why does this need to be a CHECK() rather than a DCHECK()? http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:113: model_, model_index, item_id); Nit: As mentioned previously, if you pass "item_id + model_index" as the last arg here, you can omit the final increment in this loop (and I think the code is clearer). http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.h (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:48: // BrowserActionsHostDelegate overrides: Nit: Is this supposed to refer to BroweserActionView::Delegate? http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:55: // DragController overrides: Nit: Lump these with the delegate overrides above (I would probably list these first, actually) as this class doesn't directly inherit from DragController. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:70: views::MenuItemView* AddBookmarkMenuItem(views::MenuItemView* parent, Nit: Comment this, in particular what happens to the args http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:88: typedef std::vector<linked_ptr<BrowserActionView> > BrowserActionViews; As noted, this should be converted to ScopedVector; http://crbug.com/137767 is now on file to eliminate most or all linked_ptr<> usage in the codebase. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.cc (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:37: AddChildView(button_); Nit: Consider adding children in response to this object itself being added to a view hierarchy; this is the typical pattern in views and it avoids the need to allocate anything if for some reason you're constructed but never put in a hierarchy. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:42: RemoveChildView(button_); If you're trying to manage the |button_| lifetime yourself, omit this line and instead use set_parent_owned(true) when you add the button as a child. This is not only clearer about ownership, it prevents problems if for some reason someone else removes the button as your child. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:54: new gfx::Canvas(gfx::ImageSkiaRep(icon, ui::SCALE_FACTOR_100P), false); Is explicitly giving the scale as 100 correct? http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:119: delegate_->GetBrowser()->profile()->GetOriginalProfile())); Nit: Can factor out this entire last argument as a temp http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:415: string16 name = UTF8ToUTF16(browser_action()->GetTitle(tab_id)); Nit: Simpler: std::string name = browser_action()->GetTitle(tab_id); return UTF8ToUTF16(name.empty() ? extension()->name() : name); http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_actions_container.cc (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_actions_container.cc:469: switch (model_->ExecuteBrowserAction(extension, browser_, &popup_url)) { Nit: Why not just: if (model_->ExecuteBrowserAction(extension, browser_, &popup_url) == ExtensionToolbarModel::ACTION_SHOW_POPUP) ShowPopup(button, popup_url); http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.cc (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.cc:86: if (!extension_service) Can either of these NULL-checks ever actually fail?
http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.cc (right): http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:59: } Can I do it later. This border is temporary, only to let me check in so other people hookup their code to this menu. I'll be implementing it later per UI specs. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.cc:1189: return GetExtensionPrefAsVector(action_box_enabled ? kExtensionActionBoxBar : On 2012/07/18 01:37:25, Peter Kasting wrote: > Nit: Break after '?' (or just after '('), not ':' (2 places) Done. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.cc:1974: iter != strings.end(); ++iter) { On 2012/07/18 01:37:25, Peter Kasting wrote: > Nit: No {} Sure, but if the goal is code readability, than multiline for statement is much more readable with {} http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.h:551: // Helper function to Get/Set array of strings from/to prefs On 2012/07/18 01:37:25, Peter Kasting wrote: > Nit: Trailing period Done. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.h:553: void SetExtensionPrefFromVector(const char* pref, On 2012/07/18 01:37:25, Peter Kasting wrote: > Nit: Args must be aligned It wont fit if I align it with the first argument http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.cc (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:224: void ExtensionToolbarModel::PopulateForActionBoxMode() { On 2012/07/18 01:37:25, Peter Kasting wrote: > Nit: Please put at least your new functions in the same definition order as > declaration order. (LGTM in advance if anything else in this file is out of > order and you want to TBR those changes separately ahead of time.) Done. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:225: const std::vector<std::string> toolbar_order = On 2012/07/18 01:37:25, Peter Kasting wrote: > Nit: There's a lot of usage of vector<string> in the header and implementation, > you should probably typedef it somewhere. Done. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:231: for (ExtensionSet::const_iterator it = service_->extensions()->begin(); On 2012/07/18 01:37:25, Peter Kasting wrote: > Tiny nit: Be consistent across the files in your change about whether you name > iterators |i|, |it|, or |iter|. (Personally I prefer |i| but it doesn't really > matter much.) Done. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:243: (action_box_pos == action_box_order.end())) { On 2012/07/18 01:37:25, Peter Kasting wrote: > Nit: No {} Done. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:267: int index = std::distance(order.begin(), pos); On 2012/07/18 01:37:25, Peter Kasting wrote: > Nit: Just use "pos - order.begin()"; inline into next statement Done. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:280: ++iter) { On 2012/07/18 01:37:25, Peter Kasting wrote: > Nit: Why is this on a separate line? Done. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:288: const std::vector<std::string>& order, ExtensionList* result_list) { On 2012/07/18 01:37:25, Peter Kasting wrote: > Nit: One arg per line Done. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:340: ids.clear(); On 2012/07/18 01:37:25, Peter Kasting wrote: > Nit: Use a new temp instead of clearing the old one. Done. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:378: return action_box_menu_items_[index]; On 2012/07/18 01:37:25, Peter Kasting wrote: > Nit: Why not inline this as a unix_hacker()-style function like you did with the > size accessor? (The same could be done with GetExtensionByIndex()) Done. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/action_box_menu_model.cc (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.cc:11: static const int kFirstExtensionCommandId = 1000; On 2012/07/18 01:37:25, Peter Kasting wrote: > Nit: Comment on whether and how this interacts with other command ID ranges > reserved in various headers; it's not obvious as a reader that using the values > around 1000 is inherently safe. Done. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/action_box_menu_model.h (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:38: // Overridden for both ButtonMenuItemModel::Delegate and SimpleMenuModel. On 2012/07/18 01:37:25, Peter Kasting wrote: > Nit: This class does not subclass ButtonMenuItemModel::Delegate, what is this > part of the comment about? Done. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.cc (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:34: CHECK(!root_); On 2012/07/18 01:37:25, Peter Kasting wrote: > Nit: Why does this need to be a CHECK() rather than a DCHECK()? I guess because another reviewer asked me to change it to CHECK() http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:113: model_, model_index, item_id); On 2012/07/18 01:37:25, Peter Kasting wrote: > Nit: As mentioned previously, if you pass "item_id + model_index" as the last > arg here, you can omit the final increment in this loop (and I think the code is > clearer). Done. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.h (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:48: // BrowserActionsHostDelegate overrides: On 2012/07/18 01:37:25, Peter Kasting wrote: > Nit: Is this supposed to refer to BroweserActionView::Delegate? Done. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:55: // DragController overrides: On 2012/07/18 01:37:25, Peter Kasting wrote: > Nit: Lump these with the delegate overrides above (I would probably list these > first, actually) as this class doesn't directly inherit from DragController. Done. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:70: views::MenuItemView* AddBookmarkMenuItem(views::MenuItemView* parent, On 2012/07/18 01:37:25, Peter Kasting wrote: > Nit: Comment this, in particular what happens to the args Done. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:88: typedef std::vector<linked_ptr<BrowserActionView> > BrowserActionViews; On 2012/07/18 01:37:25, Peter Kasting wrote: > As noted, this should be converted to ScopedVector; http://crbug.com/137767 is > now on file to eliminate most or all linked_ptr<> usage in the codebase. Done. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.cc (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:37: AddChildView(button_); On 2012/07/18 01:37:25, Peter Kasting wrote: > Nit: Consider adding children in response to this object itself being added to a > view hierarchy; this is the typical pattern in views and it avoids the need to > allocate anything if for some reason you're constructed but never put in a > hierarchy. Done. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:42: RemoveChildView(button_); On 2012/07/18 01:37:25, Peter Kasting wrote: > If you're trying to manage the |button_| lifetime yourself, omit this line and > instead use set_parent_owned(true) when you add the button as a child. This is > not only clearer about ownership, it prevents problems if for some reason > someone else removes the button as your child. Apparently it is owned by parent by default. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:54: new gfx::Canvas(gfx::ImageSkiaRep(icon, ui::SCALE_FACTOR_100P), false); On 2012/07/18 01:37:25, Peter Kasting wrote: > Is explicitly giving the scale as 100 correct? I think so, it was an existing code, I just changed order of classes http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:119: delegate_->GetBrowser()->profile()->GetOriginalProfile())); On 2012/07/18 01:37:25, Peter Kasting wrote: > Nit: Can factor out this entire last argument as a temp Done. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:415: string16 name = UTF8ToUTF16(browser_action()->GetTitle(tab_id)); On 2012/07/18 01:37:25, Peter Kasting wrote: > Nit: Simpler: > > std::string name = browser_action()->GetTitle(tab_id); > return UTF8ToUTF16(name.empty() ? extension()->name() : name); Done. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_actions_container.cc (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_actions_container.cc:469: switch (model_->ExecuteBrowserAction(extension, browser_, &popup_url)) { On 2012/07/18 01:37:25, Peter Kasting wrote: > Nit: Why not just: > > if (model_->ExecuteBrowserAction(extension, browser_, &popup_url) == > ExtensionToolbarModel::ACTION_SHOW_POPUP) > ShowPopup(button, popup_url); I just moved this function to match h file order, I didnt modified it or even read :) http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.cc (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.cc:86: if (!extension_service) On 2012/07/18 01:37:25, Peter Kasting wrote: > Can either of these NULL-checks ever actually fail? I dont know enough yet to say for sure, but there are 7 other similar places in code that do the same check (extension_service). I do see browser NULL at time of this class creation. I dont know if user can click this button fast enough, probably not, but I think better not to gamble.
http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.cc (right): http://codereview.chromium.org/10533086/diff/18015/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:59: } On 2012/07/18 23:18:13, yefimt wrote: > Can I do it later. This border is temporary, only to let me check in so other > people hookup their code to this menu. > I'll be implementing it later per UI specs. Yes, doing it later is fine. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.h:553: void SetExtensionPrefFromVector(const char* pref, On 2012/07/18 23:18:13, yefimt wrote: > On 2012/07/18 01:37:25, Peter Kasting wrote: > > Nit: Args must be aligned > > It wont fit if I align it with the first argument That's why in this case the first argument must itself be placed on a new line, indented 4, just as you've already done with the second. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.cc (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:34: CHECK(!root_); On 2012/07/18 23:18:13, yefimt wrote: > On 2012/07/18 01:37:25, Peter Kasting wrote: > > Nit: Why does this need to be a CHECK() rather than a DCHECK()? > > I guess because another reviewer asked me to change it to CHECK() CHECKs are unusual -- normally they're used in situations where: (a) We're trying to track down a crash by converting DCHECKs to CHECKs to see which of our assertions fail, or (b) If a particular condition doesn't hold, we must crash the browser immediately rather than proceed, because we will otherwise have a critical security hole, or whatever. In this case it doesn't seem like either of these apply, so I would probably use a DCHECK unless I'm missing something. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.cc (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:42: RemoveChildView(button_); On 2012/07/18 23:18:13, yefimt wrote: > On 2012/07/18 01:37:25, Peter Kasting wrote: > > If you're trying to manage the |button_| lifetime yourself, omit this line and > > instead use set_parent_owned(true) when you add the button as a child. This > is > > not only clearer about ownership, it prevents problems if for some reason > > someone else removes the button as your child. > > Apparently it is owned by parent by default. I'm sorry, I misspoke: set_parent_owned(false). The difference between the two is rather critical :) http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_actions_container.cc (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_actions_container.cc:469: switch (model_->ExecuteBrowserAction(extension, browser_, &popup_url)) { On 2012/07/18 23:18:13, yefimt wrote: > On 2012/07/18 01:37:25, Peter Kasting wrote: > > Nit: Why not just: > > > > if (model_->ExecuteBrowserAction(extension, browser_, &popup_url) == > > ExtensionToolbarModel::ACTION_SHOW_POPUP) > > ShowPopup(button, popup_url); > > I just moved this function to match h file order, I didnt modified it or even > read :) Consider this a suggestion to go ahead and change it while you're touching it then -- that's how cleanups happen in Chromium code :) http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.cc (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.cc:86: if (!extension_service) On 2012/07/18 23:18:13, yefimt wrote: > On 2012/07/18 01:37:25, Peter Kasting wrote: > > Can either of these NULL-checks ever actually fail? > > I dont know enough yet to say for sure, but there are 7 other similar places in > code that do the same check (extension_service). The extension service might be able to be NULL when unit-testing. Is this particular code unittested? I'd be very surprised if it is. If you're still unsure, check with someone who wrote/owns/works on ExtensionService. We should NULL-check if and only if NULL is actually possible. > I do see browser NULL at time of this class creation. I dont know if user can > click this button fast enough, probably not, but I think better not to gamble. I am surprised that GetBrowser() returns NULL at that point because we ought to have a Browser before this class is created -- that's what ultimately owns the location bar. This suggests -- as I noted before -- that the right thing to do is to pass the Browser to the constructor explicitly instead of passing in some sort of delegate object that muddies the lifetime picture.
On Thu, Jul 19, 2012 at 1:27 AM, <pkasting@chromium.org> wrote: > http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... > File chrome/browser/ui/views/browser_action_view.cc (right): > > http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... > chrome/browser/ui/views/browser_action_view.cc:42: > RemoveChildView(button_); > On 2012/07/18 23:18:13, yefimt wrote: >> >> On 2012/07/18 01:37:25, Peter Kasting wrote: >> > If you're trying to manage the |button_| lifetime yourself, omit > > this line and >> >> > instead use set_parent_owned(true) when you add the button as a > > child. This >> >> is >> > not only clearer about ownership, it prevents problems if for some > > reason >> >> > someone else removes the button as your child. > > >> Apparently it is owned by parent by default. > > > I'm sorry, I misspoke: set_parent_owned(false). The difference between > the two is rather critical :) > If client wants to manage the life cyle of views::Link* itself, for example, it can/should use link_->set_owned_by_client(). Otherwise, if link_ is added to the views hierarchy it'll be managed by the framework and deleted there. -- Thiago
http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.cc (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:42: RemoveChildView(button_); On 2012/07/19 04:27:31, Peter Kasting wrote: > On 2012/07/18 23:18:13, yefimt wrote: > > On 2012/07/18 01:37:25, Peter Kasting wrote: > > > If you're trying to manage the |button_| lifetime yourself, omit this line > and > > > instead use set_parent_owned(true) when you add the button as a child. This > > is > > > not only clearer about ownership, it prevents problems if for some reason > > > someone else removes the button as your child. > > > > Apparently it is owned by parent by default. > > I'm sorry, I misspoke: set_parent_owned(false). The difference between the two > is rather critical :) Ah, as Thiago points out, the name of this has apparently changed -- it's now a zero-arg "set_owned_by_client()" call.
Hi Yefim! Porting some of that stuff to OsX, Kait and I noticed a potential problem with the action box menu model lifecycle that you may want to check. Cheers! Philippe http://codereview.chromium.org/10533086/diff/54001/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.cc (right): http://codereview.chromium.org/10533086/diff/54001/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.cc:94: action_box_menu_.reset(new ActionBoxMenu(browser, &model, bookmark_state_)); You're passing a pointer to a short-lived object into the persistent action_box_menu_ here and the pointer is saved by action_box_menu_. This could easily lead to a use-after-free.
http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.h:553: void SetExtensionPrefFromVector(const char* pref, On 2012/07/19 04:27:31, Peter Kasting wrote: > On 2012/07/18 23:18:13, yefimt wrote: > > On 2012/07/18 01:37:25, Peter Kasting wrote: > > > Nit: Args must be aligned > > > > It wont fit if I align it with the first argument > > That's why in this case the first argument must itself be placed on a new line, > indented 4, just as you've already done with the second. Done. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.cc (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:34: CHECK(!root_); On 2012/07/19 04:27:31, Peter Kasting wrote: > On 2012/07/18 23:18:13, yefimt wrote: > > On 2012/07/18 01:37:25, Peter Kasting wrote: > > > Nit: Why does this need to be a CHECK() rather than a DCHECK()? > > > > I guess because another reviewer asked me to change it to CHECK() > > CHECKs are unusual -- normally they're used in situations where: > (a) We're trying to track down a crash by converting DCHECKs to CHECKs to see > which of our assertions fail, or > (b) If a particular condition doesn't hold, we must crash the browser > immediately rather than proceed, because we will otherwise have a critical > security hole, or whatever. > > In this case it doesn't seem like either of these apply, so I would probably use > a DCHECK unless I'm missing something. Done. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.cc (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:42: RemoveChildView(button_); On 2012/07/19 04:42:35, Peter Kasting wrote: > On 2012/07/19 04:27:31, Peter Kasting wrote: > > On 2012/07/18 23:18:13, yefimt wrote: > > > On 2012/07/18 01:37:25, Peter Kasting wrote: > > > > If you're trying to manage the |button_| lifetime yourself, omit this line > > and > > > > instead use set_parent_owned(true) when you add the button as a child. > This > > > is > > > > not only clearer about ownership, it prevents problems if for some reason > > > > someone else removes the button as your child. > > > > > > Apparently it is owned by parent by default. > > > > I'm sorry, I misspoke: set_parent_owned(false). The difference between the > two > > is rather critical :) > > Ah, as Thiago points out, the name of this has apparently changed -- it's now a > zero-arg "set_owned_by_client()" call. Yes, I saw it, and changed code accordingly http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_actions_container.cc (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_actions_container.cc:469: switch (model_->ExecuteBrowserAction(extension, browser_, &popup_url)) { On 2012/07/19 04:27:31, Peter Kasting wrote: > On 2012/07/18 23:18:13, yefimt wrote: > > On 2012/07/18 01:37:25, Peter Kasting wrote: > > > Nit: Why not just: > > > > > > if (model_->ExecuteBrowserAction(extension, browser_, &popup_url) == > > > ExtensionToolbarModel::ACTION_SHOW_POPUP) > > > ShowPopup(button, popup_url); > > > > I just moved this function to match h file order, I didnt modified it or even > > read :) > > Consider this a suggestion to go ahead and change it while you're touching it > then -- that's how cleanups happen in Chromium code :) Done. http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.cc (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.cc:86: if (!extension_service) On 2012/07/19 04:27:31, Peter Kasting wrote: > On 2012/07/18 23:18:13, yefimt wrote: > > On 2012/07/18 01:37:25, Peter Kasting wrote: > > > Can either of these NULL-checks ever actually fail? > > > > I dont know enough yet to say for sure, but there are 7 other similar places > in > > code that do the same check (extension_service). > > The extension service might be able to be NULL when unit-testing. Is this > particular code unittested? I'd be very surprised if it is. > > If you're still unsure, check with someone who wrote/owns/works on > ExtensionService. We should NULL-check if and only if NULL is actually > possible. > > > I do see browser NULL at time of this class creation. I dont know if user can > > click this button fast enough, probably not, but I think better not to gamble. > > I am surprised that GetBrowser() returns NULL at that point because we ought to > have a Browser before this class is created -- that's what ultimately owns the > location bar. This suggests -- as I noted before -- that the right thing to do > is to pass the Browser to the constructor explicitly instead of passing in some > sort of delegate object that muddies the lifetime picture. It was my original implementation to pass browser to constructor. But it is NULL at the time of button creation. Maybe it is the way location bar getting browser (it gets WebContents and then FindBrowserWithWebContents()). At the time button created WebContents is NULL. http://codereview.chromium.org/10533086/diff/54001/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.cc (right): http://codereview.chromium.org/10533086/diff/54001/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.cc:94: action_box_menu_.reset(new ActionBoxMenu(browser, &model, bookmark_state_)); On 2012/07/19 15:33:50, beaudoin wrote: > You're passing a pointer to a short-lived object into the persistent > action_box_menu_ here and the pointer is saved by action_box_menu_. This could > easily lead to a use-after-free. Menu gets destroyed before model, but it bring a good point, why menu is a class member. There is need for that, fixed.
http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.cc (right): http://codereview.chromium.org/10533086/diff/40002/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.cc:86: if (!extension_service) On 2012/07/19 20:00:15, yefimt wrote: > It was my original implementation to pass browser to constructor. But it is NULL > at the time of button creation. Maybe it is the way location bar getting browser > (it gets WebContents and then FindBrowserWithWebContents()). At the time button > created WebContents is NULL. The Browser isn't NULL at this point, the LocationBarView is simply trying to get it in a way that isn't very good. You can see if you look in ToolbarView that where |location_bar_| is created we have a non-NULL |browser_|. What you should do is pass that directly to the LocationBarView and have it cache it. (This will also let you stop passing in the various arguments calculated on the caller side that use |browser_| in one way or another; the LocationBarView can do those calculations in the constructor as needed.) Then the LocationBarView can get rid of GetBrowserFromDelegate() and you can eliminate your new delegate entirely.
I understand what you are saying, just I feel that it is out of scope of this CL, especially when people waiting for it so they can hook up their code. I can try to do it in a separate CL, but I when looked at creation chain of LocationBarView, it is very long. Just saying it is not trivial change.
On 2012/07/19 21:16:17, yefimt wrote: > I understand what you are saying, just I feel that it is out of scope of this > CL, especially when people waiting for it so they can hook up their code. > I can try to do it in a separate CL, but I when looked at creation chain of > LocationBarView, it is very long. > Just saying it is not trivial change. What do you mean, "the creation chain of LocationBarView is very long"? Is there more to this change than what I just described? ToolbarView is the direct owner and creator of LocationBarView, changing that should be sufficient here. It seems from here like it should be very simple to make this set of changes, and getting rid of these delegate objects you're introducing will make the overall change noticeably simpler. I am not keen on checking in something that isn't the best solution possible, even as an intermediary step.
talked to nkostylev@, it is OK to pass NULL instead of Browser in SimpleWebViewDialog and action box button should not be shown
lgtm
Most of my remaining concerns are in extension_toolbar_model.cc -- previously I didn't look closely at the logic there and mainly reviewed for style, but I did a more thorough review this time and at least a few things worry me. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.cc:759: for (unsigned int i = 0; i < remove_pref_ids.size(); ++i) { Tiny nit: While you're touching this file can you change this to size_t? http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.h:106: std::vector<std::string> GetToolbarOrder(); Nit: The actual implementation of this uses ExtensionIdSet, which is already a public typedef to vector<string>. Can you make [Get,Set][Toolbar,ActionBox]Order() (and the new helpers they use) consistently use ExtensionIdSet everywhere they currently use vector<string> (in both the header and .cc file)? This will allow you to use this same typedef in extension_toolbar_model.* instead of defining a new one that has to match the type used here. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.h:108: // Set the order that the browser actions appear in the toolbar. Nit: Eliminate this comment and the blank line above in favor of using "Get/Set" like you have in the comment for [Get,Set]ActionBoxOrder(). http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.cc (right): http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:229: void ExtensionToolbarModel::PopulateForActionBoxMode() { This function never calls GetBrowserActionVisibility() anywhere, where PopulateForNonActionBoxMode() does. That seems questionable? Is there a problem here? http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:235: // Create the lists. Nit: This comment is misleading, because we don't create the lists until the next block. Instead, this should be something like "Add all browser actions not already in the toolbar or action box to the action box." But I question whether that's the right thing to do. Shouldn't GetActionBoxOrder() be responsible for doing something like this? http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:246: extension->id()); Nit: Indent even with first arg in line just above http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:256: for (size_t i = 0; i < toolbar_items_.size(); i++) { This loop seems wrong for a couple of reasons. (1) I don't think |action_box_menu_items_| is guaranteed to be the same length as |toolbar_items_| (especially after the loop above!), so it doesn't seem safe to blindly index into one based on the size of the other. This should probably be two loops. (2) Are the items in these two lists guaranteed to be fully disjoint? If not, we'll notify about some of them twice. Seems bad. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:258: BrowserActionAdded(action_box_menu_items_[i], i)); Maybe not in scope for this change, but it seems like maybe BrowserActionAdded() should pass along whether an extension is in the action box or the toolbar, or there should be a separate observer function for these two cases? I guess I don't know what the observers ultimately do with this, but it seems like if it's important that we pass along the order number to the observer, then this would be important too... http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:277: if (!extension->browser_action()) Nit: Can combine with the next condition by adding "||" http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:285: MergeLists(sorted, unsorted, &toolbar_items_); Nit: This function is only called once, and is short and simple enough that I probably wouldn't factor it out into a helper. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:297: ExtensionList* unsorted) { Nit: I suggest: DCHECK_EQ(order.size(), sorted->size()); ...atop this function to make it clear that the offset access below is safe. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:300: if (pos != order.end()) { Nit: No {} needed http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:313: if (*iter != NULL) Is this check actually necessary? It seems like it could only succeed if GetToolbarOrder() returns a list that has elements in it that don't actually appear in ExtensionService::extensions(). That seems like it shouldn't happen. If this isn't necessary, then this whole function can be eliminated, and the caller can simply append |unsorted| to the end of |sorted| and then notify. (I know the origin of this is from the original code; if it's difficult to determine the answer to this you can do it as a followup, but file a bug for it if so.) http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:314: result_list->push_back(*iter); This seems to assume |result_list| was empty on entry. I suggest DCHECKing that. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:326: if (!extension) Can this happen? This seems like another case that should only happen if the order lists have entries that aren't valid extensions. Either this should be eliminated, be a DCHECK(extension), or if this case really can happen, then the code should probably just skip the item and not return entirely: if (extension) result_list->push_back(extension); http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:338: for (ExtensionList::iterator iter = begin(); iter != end(); ++iter) Nit: I think this can be a const_iterator (and below, if you choose to change that one, see comment there) http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:344: for (size_t i = 0; i < action_box_menu_items_.size(); ++i) Nit: Is there a reason you use an iterator above and an index here? I think it'd be better to be consistent (I'd use an iterator for both). http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.h (right): http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.h:21: typedef std::vector<std::string> VectorOfStrings; Nit: As noted in extension_prefs.h, if you change the functions there to consistently use the pre-existing ExtensionIdSet type, you can eliminate this typedef and use that type instead in this header/.cc file. (This will require #including extension_prefs.h here, which seems OK to me.) http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.h:89: const extensions::Extension* GetExtensionByIndex(int index) const { Nit: Inlined functions should be named unix_hacker()-style. Normally we'd also name them without "get" and as close to the actual variable names in question as possible, a la: toolbar_item_for_index() action_box_menu_item_for_index() (2 places) http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.cc (right): http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:9: #endif Nit: All #ifdefed #includes should go below all the non-ifdefed includes, as opposed to being inserted at their normal locations. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:65: #if defined(OS_WIN) Nit: I'd #ifdef as little as possible, and add a TODO for the non-Win case (may want a bug for this too): #if defined(OS_WIN) SkColor border_color = ...; #else // TODO: Use correct theme color on non-Windows. SkColor border_color = SK_ColorBLACK; // Note constant #endif return views::Border::CreateSolidBorder(1, border_color); (2 places) http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:67: ui::NativeThemeWin::instance()->GetThemeColorWithDefault( Not necessary for this change, but it would be nice in the future to find a way to make use of the same code for both here and menu_item_view_win.cc, in as many places as are relevant. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.cc (right): http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:52: button_ = new BrowserActionButton(extension, browser_, delegate_); This leaks if you're never added to a view hierarchy. Do this in ViewHierarchyChanged() where you AddChildView() instead. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:140: Profile* original_profile = browser_->profile()->GetOriginalProfile(); Nit: You can even make this temp a NotificationSource and pull the "content::Source<Profile>()" bit into it. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:200: void BrowserActionButton::ShowContextMenuForView(View* source, Nit: This function and ShowContextMenu() are exact copies. If they're truly identical we should just have one (and if we need two overrides, have one call the other). It's suspicious to me that neither this function nor the other pay attention to any of their arguments. Is this really correct? http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_actions_container.h (right): http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_actions_container.h:195: virtual Browser* GetBrowser() const; This should be removed, as it's no longer overriding anything. BrowserActionOverflowMenuController can take the Browser as a constructor argument to avoid reintroducing the browser() accessor on this class. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.cc (right): http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.cc:85: if (!extension_service) Nit: My question several patchsets ago about whether this can actually be NULL here never got answered. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/location_bar_view.h:449: // Browser object associated with LocationBarView (could be NULL). Nit: Expand this comment slightly: // The Browser this LocationBarView is in. Note that at least // chromeos::SimpleWebViewDialog uses a LocationBarView outside any browser // window, so this may be NULL.
http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.cc (right): http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.cc:85: if (!extension_service) I think we can assume ES will never be NULL here.
http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.cc:759: for (unsigned int i = 0; i < remove_pref_ids.size(); ++i) { On 2012/07/21 01:55:13, Peter Kasting wrote: > Tiny nit: While you're touching this file can you change this to size_t? Done. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.h:106: std::vector<std::string> GetToolbarOrder(); On 2012/07/21 01:55:13, Peter Kasting wrote: > Nit: The actual implementation of this uses ExtensionIdSet, which is already a > public typedef to vector<string>. Can you make > [Get,Set][Toolbar,ActionBox]Order() (and the new helpers they use) consistently > use ExtensionIdSet everywhere they currently use vector<string> (in both the > header and .cc file)? This will allow you to use this same typedef in > extension_toolbar_model.* instead of defining a new one that has to match the > type used here. Done. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.h:108: // Set the order that the browser actions appear in the toolbar. On 2012/07/21 01:55:13, Peter Kasting wrote: > Nit: Eliminate this comment and the blank line above in favor of using "Get/Set" > like you have in the comment for [Get,Set]ActionBoxOrder(). Done. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.cc (right): http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:229: void ExtensionToolbarModel::PopulateForActionBoxMode() { On 2012/07/21 01:55:13, Peter Kasting wrote: > This function never calls GetBrowserActionVisibility() anywhere, where > PopulateForNonActionBoxMode() does. That seems questionable? Is there a > problem here? No, Aaron advised that just using toolbar and action box orders should be enough. We agreed that I'll clean up rest of the files in a separate CL. And then after action box will be enabled by default, non action box functionality will be removed http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:235: // Create the lists. On 2012/07/21 01:55:13, Peter Kasting wrote: > Nit: This comment is misleading, because we don't create the lists until the > next block. > > Instead, this should be something like "Add all browser actions not already in > the toolbar or action box to the action box." > > But I question whether that's the right thing to do. Shouldn't > GetActionBoxOrder() be responsible for doing something like this? Comment - done. I dont think GetActionBoxOrder() should do it, but you are right that changes in extensions are not handled when action box is enabled. Fixing it in Observe() http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:246: extension->id()); On 2012/07/21 01:55:13, Peter Kasting wrote: > Nit: Indent even with first arg in line just above Done. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:256: for (size_t i = 0; i < toolbar_items_.size(); i++) { On 2012/07/21 01:55:13, Peter Kasting wrote: > This loop seems wrong for a couple of reasons. > > (1) I don't think |action_box_menu_items_| is guaranteed to be the same length > as |toolbar_items_| (especially after the loop above!), so it doesn't seem safe > to blindly index into one based on the size of the other. This should probably > be two loops. > (2) Are the items in these two lists guaranteed to be fully disjoint? If not, > we'll notify about some of them twice. Seems bad. Yes an extension guarantied to be in the one list only http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:258: BrowserActionAdded(action_box_menu_items_[i], i)); On 2012/07/21 01:55:13, Peter Kasting wrote: > Maybe not in scope for this change, but it seems like maybe BrowserActionAdded() > should pass along whether an extension is in the action box or the toolbar, or > there should be a separate observer function for these two cases? I guess I > don't know what the observers ultimately do with this, but it seems like if it's > important that we pass along the order number to the observer, then this would > be important too... I see your point. I'll keep it in mind when get to the rest of functionality http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:277: if (!extension->browser_action()) On 2012/07/21 01:55:13, Peter Kasting wrote: > Nit: Can combine with the next condition by adding "||" I thought it is more readable this way and second one eventually will go away completely. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:285: MergeLists(sorted, unsorted, &toolbar_items_); On 2012/07/21 01:55:13, Peter Kasting wrote: > Nit: This function is only called once, and is short and simple enough that I > probably wouldn't factor it out into a helper. Done. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:285: MergeLists(sorted, unsorted, &toolbar_items_); On 2012/07/21 01:55:13, Peter Kasting wrote: > Nit: This function is only called once, and is short and simple enough that I > probably wouldn't factor it out into a helper. Done. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:297: ExtensionList* unsorted) { On 2012/07/21 01:55:13, Peter Kasting wrote: > Nit: I suggest: > > DCHECK_EQ(order.size(), sorted->size()); > > ...atop this function to make it clear that the offset access below is safe. Done. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:300: if (pos != order.end()) { On 2012/07/21 01:55:13, Peter Kasting wrote: > Nit: No {} needed Done. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:313: if (*iter != NULL) On 2012/07/21 01:55:13, Peter Kasting wrote: > Is this check actually necessary? It seems like it could only succeed if > GetToolbarOrder() returns a list that has elements in it that don't actually > appear in ExtensionService::extensions(). That seems like it shouldn't happen. > > If this isn't necessary, then this whole function can be eliminated, and the > caller can simply append |unsorted| to the end of |sorted| and then notify. > > (I know the origin of this is from the original code; if it's difficult to > determine the answer to this you can do it as a followup, but file a bug for it > if so.) I think you are right, I cannot see how it could be NULL. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:314: result_list->push_back(*iter); On 2012/07/21 01:55:13, Peter Kasting wrote: > This seems to assume |result_list| was empty on entry. I suggest DCHECKing > that. Done. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:326: if (!extension) On 2012/07/21 01:55:13, Peter Kasting wrote: > Can this happen? This seems like another case that should only happen if the > order lists have entries that aren't valid extensions. > > Either this should be eliminated, be a DCHECK(extension), or if this case really > can happen, then the code should probably just skip the item and not return > entirely: > > if (extension) > result_list->push_back(extension); I think it could happen. Extensions order coming from preferences, it could be edited or corrupted. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:338: for (ExtensionList::iterator iter = begin(); iter != end(); ++iter) On 2012/07/21 01:55:13, Peter Kasting wrote: > Nit: I think this can be a const_iterator (and below, if you choose to change > that one, see comment there) Done. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:344: for (size_t i = 0; i < action_box_menu_items_.size(); ++i) On 2012/07/21 01:55:13, Peter Kasting wrote: > Nit: Is there a reason you use an iterator above and an index here? I think > it'd be better to be consistent (I'd use an iterator for both). Done. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.h (right): http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.h:21: typedef std::vector<std::string> VectorOfStrings; On 2012/07/21 01:55:13, Peter Kasting wrote: > Nit: As noted in extension_prefs.h, if you change the functions there to > consistently use the pre-existing ExtensionIdSet type, you can eliminate this > typedef and use that type instead in this header/.cc file. (This will require > #including extension_prefs.h here, which seems OK to me.) Done. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.h:89: const extensions::Extension* GetExtensionByIndex(int index) const { On 2012/07/21 01:55:13, Peter Kasting wrote: > Nit: Inlined functions should be named unix_hacker()-style. > > Normally we'd also name them without "get" and as close to the actual variable > names in question as possible, a la: > > toolbar_item_for_index() > action_box_menu_item_for_index() > > (2 places) Yes, but they not returning a member variable but and expression with it, and renaming this function will cause a chain reaction:) I'll need to touch 2 other files, then you will ask me to fix them and because they are mac and linux specific, I cannot even verify they compile. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.cc (right): http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:9: #endif On 2012/07/21 01:55:13, Peter Kasting wrote: > Nit: All #ifdefed #includes should go below all the non-ifdefed includes, as > opposed to being inserted at their normal locations. Done. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:65: #if defined(OS_WIN) On 2012/07/21 01:55:13, Peter Kasting wrote: > Nit: I'd #ifdef as little as possible, and add a TODO for the non-Win case (may > want a bug for this too): > > #if defined(OS_WIN) > SkColor border_color = ...; > #else > // TODO: Use correct theme color on non-Windows. > SkColor border_color = SK_ColorBLACK; // Note constant > #endif > return views::Border::CreateSolidBorder(1, border_color); > > (2 places) Done. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:67: ui::NativeThemeWin::instance()->GetThemeColorWithDefault( On 2012/07/21 01:55:13, Peter Kasting wrote: > Not necessary for this change, but it would be nice in the future to find a way > to make use of the same code for both here and menu_item_view_win.cc, in as many > places as are relevant. OK http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.cc (right): http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:52: button_ = new BrowserActionButton(extension, browser_, delegate_); On 2012/07/21 01:55:13, Peter Kasting wrote: > This leaks if you're never added to a view hierarchy. Do this in > ViewHierarchyChanged() where you AddChildView() instead. It is difficult to do because when BrowserActionView created caller sets a couple settings to BrowserActionButton. Postponing button creation will force BrowserActionView to cache any possible button setting. So reverting to view owned by client. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:140: Profile* original_profile = browser_->profile()->GetOriginalProfile(); On 2012/07/21 01:55:13, Peter Kasting wrote: > Nit: You can even make this temp a NotificationSource and pull the > "content::Source<Profile>()" bit into it. Done. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:200: void BrowserActionButton::ShowContextMenuForView(View* source, On 2012/07/21 01:55:13, Peter Kasting wrote: > Nit: This function and ShowContextMenu() are exact copies. If they're truly > identical we should just have one (and if we need two overrides, have one call > the other). > > It's suspicious to me that neither this function nor the other pay attention to > any of their arguments. Is this really correct? I cannot comment on correctness. Moved body to a 3rd function. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_actions_container.h (right): http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_actions_container.h:195: virtual Browser* GetBrowser() const; On 2012/07/21 01:55:13, Peter Kasting wrote: > This should be removed, as it's no longer overriding anything. > > BrowserActionOverflowMenuController can take the Browser as a constructor > argument to avoid reintroducing the browser() accessor on this class. Done. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.cc (right): http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.cc:85: if (!extension_service) On 2012/07/23 22:55:23, Aaron Boodman wrote: > I think we can assume ES will never be NULL here. Done. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/location_bar_view.h:449: // Browser object associated with LocationBarView (could be NULL). On 2012/07/21 01:55:13, Peter Kasting wrote: > Nit: Expand this comment slightly: > > // The Browser this LocationBarView is in. Note that at least > // chromeos::SimpleWebViewDialog uses a LocationBarView outside any browser > // window, so this may be NULL. Done.
http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.cc (right): http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:313: if (*iter != NULL) On 2012/07/21 01:55:13, Peter Kasting wrote: > Is this check actually necessary? It seems like it could only succeed if > GetToolbarOrder() returns a list that has elements in it that don't actually > appear in ExtensionService::extensions(). That seems like it shouldn't happen. This can happen due to sync. The order is synced across machines, but sometimes a machine won't necessarily have the extensions loaded that are specified here.
http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.cc (right): http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:313: if (*iter != NULL) On 2012/07/23 23:57:51, Aaron Boodman wrote: > On 2012/07/21 01:55:13, Peter Kasting wrote: > > Is this check actually necessary? It seems like it could only succeed if > > GetToolbarOrder() returns a list that has elements in it that don't actually > > appear in ExtensionService::extensions(). That seems like it shouldn't > happen. > > This can happen due to sync. The order is synced across machines, but sometimes > a machine won't necessarily have the extensions loaded that are specified here. OK, restored original code
http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.cc (right): http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:313: if (*iter != NULL) On 2012/07/23 23:57:51, Aaron Boodman wrote: > On 2012/07/21 01:55:13, Peter Kasting wrote: > > Is this check actually necessary? It seems like it could only succeed if > > GetToolbarOrder() returns a list that has elements in it that don't actually > > appear in ExtensionService::extensions(). That seems like it shouldn't > happen. > > This can happen due to sync. The order is synced across machines, but sometimes > a machine won't necessarily have the extensions loaded that are specified here. Thanks, that's good to know! Yefim, can you make sure to add comments about this (in the various spots I was concerned about) so the next person who reads the various places in this file I've asked about will have the benefit of Aaron's knowledge here? http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.h (right): http://codereview.chromium.org/10533086/diff/53010/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.h:89: const extensions::Extension* GetExtensionByIndex(int index) const { On 2012/07/23 23:47:52, yefimt wrote: > On 2012/07/21 01:55:13, Peter Kasting wrote: > > Nit: Inlined functions should be named unix_hacker()-style. > > > > Normally we'd also name them without "get" and as close to the actual variable > > names in question as possible, a la: > > > > toolbar_item_for_index() > > action_box_menu_item_for_index() > > > > (2 places) > > Yes, but they not returning a member variable but and expression with it, and > renaming this function will cause a chain reaction:) > I'll need to touch 2 other files, then you will ask me to fix them and because > they are mac and linux specific, I cannot even verify they compile. That's what we have trybots for. Please do at least make the name use lowercase and underscores, the actual names are up to your best judgment. http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.cc (right): http://codereview.chromium.org/10533086/diff/53010/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:52: button_ = new BrowserActionButton(extension, browser_, delegate_); On 2012/07/23 23:47:52, yefimt wrote: > On 2012/07/21 01:55:13, Peter Kasting wrote: > > This leaks if you're never added to a view hierarchy. Do this in > > ViewHierarchyChanged() where you AddChildView() instead. > > It is difficult to do because when BrowserActionView created caller sets a > couple settings to BrowserActionButton. Postponing button creation will force > BrowserActionView to cache any possible button setting. > So reverting to view owned by client. You mean the stuff in ActionBoxMenu::PopulateMenu()? Why is the caller setting state on the button? Why isn't that happening in this class instead? Seems like a layering violation.
Lots of nits... sorry if some were already discussed. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ext... File chrome/browser/extensions/extension_prefs.cc (right): https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ext... chrome/browser/extensions/extension_prefs.cc:73: // A preference that tracks order of extensions in an action box grammar nit: "the order". https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ext... chrome/browser/extensions/extension_prefs.cc:77: // A preference that tracks order of extensions in an toolbar when grammar nits: "the order", "a toolbar", and "is enabled". https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ext... chrome/browser/extensions/extension_prefs.cc:1959: std::string extension_id; nit: move string decl out of loop. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ext... File chrome/browser/extensions/extension_toolbar_model.cc (right): https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ext... chrome/browser/extensions/extension_toolbar_model.cc:145: bool found_in_toolbar = FindInExtensionList(extension, toolbar_items_); nit: Instead of using two bools here, use a single extensions::ExtensionList* |container| or |list| or similar that's NULL or points to the right container; that should simplify the code below (and the code here if you implement my suggestion in the header about returning the containing list). https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ext... chrome/browser/extensions/extension_toolbar_model.cc:153: return; // Already exists. nit: remoove inline comment, it's explained well above. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ext... chrome/browser/extensions/extension_toolbar_model.cc:157: if (service_->extension_prefs()->GetBrowserActionVisibility(extension)) nit: fit on line above as "else if" https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ext... chrome/browser/extensions/extension_toolbar_model.cc:168: // TODO: Implement this when implementing drag & drop for action box menu. nit: use TODO(ldap): format. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ext... chrome/browser/extensions/extension_toolbar_model.cc:181: ExtensionList* result_list) { nit: Name |list| instead of |result_list| and similar below. What's result-y about it? https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ext... chrome/browser/extensions/extension_toolbar_model.cc:250: // to the action box. nit: explain why the prefs list might miss some extensions. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ext... chrome/browser/extensions/extension_toolbar_model.cc:257: extensions::ExtensionPrefs::ExtensionIdSet::const_iterator toolbar_pos = nit: continue early if it's in the first list, don't search the second list; alternatively, inline the conditionals to achieve the same. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ext... chrome/browser/extensions/extension_toolbar_model.cc:270: // Inform observers. nit: Can this be built-in by calling AddExtension from FillExtensionList? https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ext... chrome/browser/extensions/extension_toolbar_model.cc:303: for (ExtensionList::const_iterator iter = sorted.begin(); nit: use toolbar_items_.insert for |sorted| (like unsorted), instead of looping. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ext... chrome/browser/extensions/extension_toolbar_model.cc:318: void ExtensionToolbarModel::AddToProperList(const Extension* extension, nit: I think if args don't fit aligned to inside the paren, they are all supposed to be indented b 4 spaces (move |extension| to the next line). https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ext... chrome/browser/extensions/extension_toolbar_model.cc:318: void ExtensionToolbarModel::AddToProperList(const Extension* extension, nit: make this a file-local function in an anon namespace. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ext... chrome/browser/extensions/extension_toolbar_model.cc:337: const extensions::Extension* extension = nit: declare a single non-const Extension* outside the loop and re-use here. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ext... chrome/browser/extensions/extension_toolbar_model.cc:339: if (!extension) nit: invert NULL check and just perform the single statement on success https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ext... chrome/browser/extensions/extension_toolbar_model.cc:341: result_list->push_back(extension); nit: Call AddExtension here instead, and remove the observer notification code from PopulateForActionBoxMode https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ext... chrome/browser/extensions/extension_toolbar_model.cc:345: bool ExtensionToolbarModel::FindInExtensionList( nit: make this a file-local function in an anon namespace. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ext... File chrome/browser/extensions/extension_toolbar_model.h (right): https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ext... chrome/browser/extensions/extension_toolbar_model.h:118: // A few helper functions to work with extension lists. nit: describe these in a bit more detail. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ext... chrome/browser/extensions/extension_toolbar_model.h:126: bool FindInExtensionList(const extensions::Extension* extension, nit: |IsInExtensionList| seems better. Or reimplement this to return a pointer to the list containing the extension (or NULL if there is none), that should save callers some hassle. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... File chrome/browser/ui/toolbar/action_box_menu_model.cc (right): https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/toolbar/action_box_menu_model.cc:12: // that shown before extensions. grammar nit: "that show" https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/toolbar/action_box_menu_model.cc:49: size_t ActionBoxMenuModel::action_box_extensions_size() { nit: definition order should match declaration order in the header. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/toolbar/action_box_menu_model.cc:54: ActionBoxMenuModel::GetActionBoxExtensionByIndex(int index) { nit: definition order should match declaration order in the header. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... File chrome/browser/ui/toolbar/action_box_menu_model.h (right): https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/toolbar/action_box_menu_model.h:10: #include "base/file_path.h" nit: is this include necessary? https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/toolbar/action_box_menu_model.h:11: #include "base/memory/scoped_ptr.h" nit: is this include necessary? https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/toolbar/action_box_menu_model.h:13: #include "ui/base/models/button_menu_item_model.h" nit: is this include necessary? https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/toolbar/action_box_menu_model.h:22: nit: remove extra blank line. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/toolbar/action_box_menu_model.h:31: // Active box extensions. nit: "Action" not "Active"? Capitalize "Box"? Remove or expand upon comment. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/toolbar/action_box_menu_model.h:38: // Overridden for both SimpleMenuModel::Delegate and SimpleMenuModel. nit: These are all actually from SimpleMenuModel::Delegate, use "Overridden from" standard language. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/toolbar/action_box_menu_model.h:39: virtual void ExecuteCommand(int command_id) OVERRIDE; nit: Match OVERRIDE declaration order and visibility (public); same for Observe. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/toolbar/action_box_menu_model.h:52: void Build(); nit: name standard |Init|? Or inline in ctor? https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... File chrome/browser/ui/views/action_box_menu.cc (right): https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/action_box_menu.cc:19: #include <vssym32.h> nit: comment as to why this is needed. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/action_box_menu.cc:41: // We have icons, set this so we get the taller menu style. nit: I prefer making statements about the objects without using "we", use impersonal verb conjugation if needed: http://en.wikipedia.org/wiki/Impersonal_verb (this general preference nit applies here, below, and elsewhere) Perhaps this particular comment isn't needed at all. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/action_box_menu.cc:50: // Ignore the result since we don't need to handle a deleted menu specially. nit: for clarity, perhaps mention that this menu isn't deletable, as that only applies to bookmark menus? https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/action_box_menu.cc:63: #if defined(OS_WIN) nit: declare and init an SkColor with SK_ColorBLACK above the preprocessor block; move the return statement with |border_color| below, and then just override the SK_ColorBLACK for OS_WIN. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/action_box_menu.cc:64: // TODO: Move to Windows only files if possible. nit: use TODO(ldap): style here and elsewhere... https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/action_box_menu.cc:77: #if defined(OS_WIN) nit: employ the pattern I suggested above here as well. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/action_box_menu.cc:135: if (model_->GetTypeAt(model_index) == ui::MenuModel::TYPE_COMMAND) { nit: Can you clarify generally what menu items are and aren't commands, and why/how they're styled and handled differently? https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/action_box_menu.cc:149: views::MenuItemView* ActionBoxMenu::AddBookmarkMenuItem( nit: the return value is unused in the sole caller, consider removing it. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/action_box_menu.cc:157: DCHECK(icon); nit: no need to DCHECK immediately before a de-reference; we'll see the crash plainly on a NULL de-ref. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... File chrome/browser/ui/views/action_box_menu.h (right): https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/action_box_menu.h:5: #ifndef CHROME_BROWSER_UI_VIEWS_ACTION_BOX_MENU_H_ nit: do these files belong in the toolbar subdir? https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/action_box_menu.h:14: #include "ui/base/models/menu_model.h" nit: is this include necessary? https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/action_box_menu.h:43: // MenuDelegate overrides: nit: Standardize terminology here with what you use elsewhere (other files). https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/action_box_menu.h:44: virtual void ExecuteCommand(int id) OVERRIDE; nit: Match OVERRIDE declaration visibility (public), same for others below. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/action_box_menu.h:48: // BrowserActionView::Delegate and DragController overrides: nit: Please differentiate OVERRIDEs by declaration, if possible. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/action_box_menu.h:67: // Creates bookmark menu item and adds it to menu, |next_id| is incremented. grammar nit: consider "Adds a new bookmark menu item to the menu, ..." https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/action_box_menu.h:71: // Populates |parent| with all the child menus in |model|. nit: what's |parent| and |model|? old args? Please update. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/action_box_menu.h:84: bool bookmark_item_state_; nit: description? https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... File chrome/browser/ui/views/browser_action_view.cc (right): https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/browser_action_view.cc:52: button_ = new BrowserActionButton(extension, browser_, delegate_); nit: move this into the initializer list, or init button_ to NULL there. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/browser_action_view.cc:72: if (tab_id >= 0) { nit: can this be merged with (or made more similar to) PaintChildren? Why use the icon width/height here, but the view width()/height() there, etc? https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/browser_action_view.cc:392: SetTooltipText(disable_tooltip_ ? string16() : GetTextForTooltip()); nit: shouldn't |disable_tooltip_| be checked in GetTextForTooltip instead? https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... File chrome/browser/ui/views/browser_action_view.h (right): https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/browser_action_view.h:28: // A single section in the browser action container. This contains the actual nit: s/section/entry/ https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/browser_action_view.h:44: // Returns relative position of the |button_| inside BrowserActionView. nit: use "button" as general terminology, as this is slightly disjoint from the actual BrowserActionView implementation. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/browser_action_view.h:80: // The container for this view. nit: it's not obvious that |delegate_| a container; perhaps clarify. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/browser_action_view.h:209: // The browser action shelf. nit: update comment. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/browser_action_view.h:221: // True if tooltip was disabled. nit: s/was/is/ https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... File chrome/browser/ui/views/location_bar/action_box_button_view.cc (right): https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/location_bar/action_box_button_view.cc:12: #include "chrome/browser/ui/browser_finder.h" nit: is this include necessary? https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/location_bar/action_box_button_view.cc:13: #include "chrome/browser/ui/tab_contents/tab_contents.h" nit: is this include necessary? https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... File chrome/browser/ui/views/location_bar/action_box_button_view.h (right): https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/location_bar/action_box_button_view.h:11: class ActionBoxMenu; nit: remove this unused forward decl. https://chromiumcodereview.appspot.com/10533086/diff/60023/chrome/browser/ui/... chrome/browser/ui/views/location_bar/action_box_button_view.h:39: bool bookmark_state_; nit: explain the significance of bookmark_state_ here.
http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.cc:73: // A preference that tracks order of extensions in an action box On 2012/07/24 23:41:55, msw wrote: > grammar nit: "the order". Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.cc:77: // A preference that tracks order of extensions in an toolbar when On 2012/07/24 23:41:55, msw wrote: > grammar nits: "the order", "a toolbar", and "is enabled". Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.cc:1959: std::string extension_id; On 2012/07/24 23:41:55, msw wrote: > nit: move string decl out of loop. Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.cc (right): http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:145: bool found_in_toolbar = FindInExtensionList(extension, toolbar_items_); On 2012/07/24 23:41:55, msw wrote: > nit: Instead of using two bools here, use a single extensions::ExtensionList* > |container| or |list| or similar that's NULL or points to the right container; > that should simplify the code below (and the code here if you implement my > suggestion in the header about returning the containing list). I believe it is much more readable as it is. Using a pointer will make portion where extension is removed from one of the list more difficult to parse. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:153: return; // Already exists. On 2012/07/24 23:41:55, msw wrote: > nit: remoove inline comment, it's explained well above. Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:157: if (service_->extension_prefs()->GetBrowserActionVisibility(extension)) On 2012/07/24 23:41:55, msw wrote: > nit: fit on line above as "else if" Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:168: // TODO: Implement this when implementing drag & drop for action box menu. On 2012/07/24 23:41:55, msw wrote: > nit: use TODO(ldap): format. Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:181: ExtensionList* result_list) { On 2012/07/24 23:41:55, msw wrote: > nit: Name |list| instead of |result_list| and similar below. What's result-y > about it? Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:250: // to the action box. On 2012/07/24 23:41:55, msw wrote: > nit: explain why the prefs list might miss some extensions. I dont think I can, it could be hundreds of reasons. For any of them we need to add these extensions to the list or user would never see these extensions. (Simplest reason preferences file was edited or corrupted). http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:257: extensions::ExtensionPrefs::ExtensionIdSet::const_iterator toolbar_pos = On 2012/07/24 23:41:55, msw wrote: > nit: continue early if it's in the first list, don't search the second list; > alternatively, inline the conditionals to achieve the same. Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:270: // Inform observers. On 2012/07/24 23:41:55, msw wrote: > nit: Can this be built-in by calling AddExtension from FillExtensionList? Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:303: for (ExtensionList::const_iterator iter = sorted.begin(); On 2012/07/24 23:41:55, msw wrote: > nit: use toolbar_items_.insert for |sorted| (like unsorted), instead of looping. Some could be NULL and need to be excluded http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:318: void ExtensionToolbarModel::AddToProperList(const Extension* extension, On 2012/07/24 23:41:55, msw wrote: > nit: make this a file-local function in an anon namespace. I removed it at all, it was used in one place only http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:337: const extensions::Extension* extension = On 2012/07/24 23:41:55, msw wrote: > nit: declare a single non-const Extension* outside the loop and re-use here. GetExtensionById() returns const Extension*, so it wont work unless i do const_cast http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:339: if (!extension) On 2012/07/24 23:41:55, msw wrote: > nit: invert NULL check and just perform the single statement on success Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:341: result_list->push_back(extension); On 2012/07/24 23:41:55, msw wrote: > nit: Call AddExtension here instead, and remove the observer notification code > from PopulateForActionBoxMode Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:345: bool ExtensionToolbarModel::FindInExtensionList( On 2012/07/24 23:41:55, msw wrote: > nit: make this a file-local function in an anon namespace. Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.h (right): http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.h:118: // A few helper functions to work with extension lists. On 2012/07/24 23:41:55, msw wrote: > nit: describe these in a bit more detail. Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.h:126: bool FindInExtensionList(const extensions::Extension* extension, On 2012/07/24 23:41:55, msw wrote: > nit: |IsInExtensionList| seems better. Or reimplement this to return a pointer > to the list containing the extension (or NULL if there is none), that should > save callers some hassle. Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/action_box_menu_model.cc (right): http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.cc:12: // that shown before extensions. On 2012/07/24 23:41:55, msw wrote: > grammar nit: "that show" Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.cc:49: size_t ActionBoxMenuModel::action_box_extensions_size() { On 2012/07/24 23:41:55, msw wrote: > nit: definition order should match declaration order in the header. Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.cc:54: ActionBoxMenuModel::GetActionBoxExtensionByIndex(int index) { On 2012/07/24 23:41:55, msw wrote: > nit: definition order should match declaration order in the header. Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/action_box_menu_model.h (right): http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:10: #include "base/file_path.h" On 2012/07/24 23:41:55, msw wrote: > nit: is this include necessary? Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:11: #include "base/memory/scoped_ptr.h" On 2012/07/24 23:41:55, msw wrote: > nit: is this include necessary? Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:13: #include "ui/base/models/button_menu_item_model.h" On 2012/07/24 23:41:55, msw wrote: > nit: is this include necessary? Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:22: On 2012/07/24 23:41:55, msw wrote: > nit: remove extra blank line. Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:31: // Active box extensions. On 2012/07/24 23:41:55, msw wrote: > nit: "Action" not "Active"? Capitalize "Box"? Remove or expand upon comment. Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:38: // Overridden for both SimpleMenuModel::Delegate and SimpleMenuModel. On 2012/07/24 23:41:55, msw wrote: > nit: These are all actually from SimpleMenuModel::Delegate, use "Overridden > from" standard language. Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:39: virtual void ExecuteCommand(int command_id) OVERRIDE; On 2012/07/24 23:41:55, msw wrote: > nit: Match OVERRIDE declaration order and visibility (public); same for Observe. Done - for order As for visibility, aa@ asked me to make them private. I dont remember if it was this place or not, but I'm sure he asked for all similar places. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:52: void Build(); On 2012/07/24 23:41:55, msw wrote: > nit: name standard |Init|? Or inline in ctor? Moved http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.cc (right): http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:19: #include <vssym32.h> On 2012/07/24 23:41:55, msw wrote: > nit: comment as to why this is needed. Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:41: // We have icons, set this so we get the taller menu style. On 2012/07/24 23:41:55, msw wrote: > nit: I prefer making statements about the objects without using "we", use > impersonal verb conjugation if needed: > http://en.wikipedia.org/wiki/Impersonal_verb > (this general preference nit applies here, below, and elsewhere) > Perhaps this particular comment isn't needed at all. Removed. As I'm new to chrome, I often use existing code as a template. In this case see wrench_menu.cc:762 :) http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:50: // Ignore the result since we don't need to handle a deleted menu specially. On 2012/07/24 23:41:55, msw wrote: > nit: for clarity, perhaps mention that this menu isn't deletable, as that only > applies to bookmark menus? Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:63: #if defined(OS_WIN) On 2012/07/24 23:41:55, msw wrote: > nit: declare and init an SkColor with SK_ColorBLACK above the preprocessor > block; move the return statement with |border_color| below, and then just > override the SK_ColorBLACK for OS_WIN. Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:64: // TODO: Move to Windows only files if possible. On 2012/07/24 23:41:55, msw wrote: > nit: use TODO(ldap): style here and elsewhere... Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:77: #if defined(OS_WIN) On 2012/07/24 23:41:55, msw wrote: > nit: employ the pattern I suggested above here as well. Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:135: if (model_->GetTypeAt(model_index) == ui::MenuModel::TYPE_COMMAND) { On 2012/07/24 23:41:55, msw wrote: > nit: Can you clarify generally what menu items are and aren't commands, and > why/how they're styled and handled differently? I did it looking at how other menus are implemented. Currently here they all command type. I guess if non command menu items will be added later they wont break this code. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:149: views::MenuItemView* ActionBoxMenu::AddBookmarkMenuItem( On 2012/07/24 23:41:55, msw wrote: > nit: the return value is unused in the sole caller, consider removing it. Would it be better to have it consistent with all MenuItemView::Add/AppendMenuItem*() functions? http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:157: DCHECK(icon); On 2012/07/24 23:41:55, msw wrote: > nit: no need to DCHECK immediately before a de-reference; we'll see the crash > plainly on a NULL de-ref. Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.h (right): http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:5: #ifndef CHROME_BROWSER_UI_VIEWS_ACTION_BOX_MENU_H_ On 2012/07/24 23:41:55, msw wrote: > nit: do these files belong in the toolbar subdir? Not sure. wrench_menu.* is in this folder too. I dont think toolbar is a right place, maybe location_bar? http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:14: #include "ui/base/models/menu_model.h" On 2012/07/24 23:41:55, msw wrote: > nit: is this include necessary? Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:44: virtual void ExecuteCommand(int id) OVERRIDE; On 2012/07/24 23:41:55, msw wrote: > nit: Match OVERRIDE declaration visibility (public), same for others below. As in other place I was asked by reviewer always make them private unless they need to be called from outside. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:48: // BrowserActionView::Delegate and DragController overrides: On 2012/07/24 23:41:55, msw wrote: > nit: Please differentiate OVERRIDEs by declaration, if possible. I was asked lump them together :) http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:67: // Creates bookmark menu item and adds it to menu, |next_id| is incremented. On 2012/07/24 23:41:55, msw wrote: > grammar nit: consider "Adds a new bookmark menu item to the menu, ..." Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:71: // Populates |parent| with all the child menus in |model|. On 2012/07/24 23:41:55, msw wrote: > nit: what's |parent| and |model|? old args? Please update. Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:84: bool bookmark_item_state_; On 2012/07/24 23:41:55, msw wrote: > nit: description? Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.cc (right): http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:52: button_ = new BrowserActionButton(extension, browser_, delegate_); On 2012/07/24 23:41:55, msw wrote: > nit: move this into the initializer list, or init button_ to NULL there. Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:72: if (tab_id >= 0) { On 2012/07/24 23:41:55, msw wrote: > nit: can this be merged with (or made more similar to) PaintChildren? Why use > the icon width/height here, but the view width()/height() there, etc? Not sure, I dont know this code enough http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.h (right): http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:28: // A single section in the browser action container. This contains the actual On 2012/07/24 23:41:55, msw wrote: > nit: s/section/entry/ Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:44: // Returns relative position of the |button_| inside BrowserActionView. On 2012/07/24 23:41:55, msw wrote: > nit: use "button" as general terminology, as this is slightly disjoint from the > actual BrowserActionView implementation. Done, but i was asked to use specific variable names:) http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:80: // The container for this view. On 2012/07/24 23:41:55, msw wrote: > nit: it's not obvious that |delegate_| a container; perhaps clarify. Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:209: // The browser action shelf. On 2012/07/24 23:41:55, msw wrote: > nit: update comment. Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:221: // True if tooltip was disabled. On 2012/07/24 23:41:55, msw wrote: > nit: s/was/is/ Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.cc (right): http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.cc:12: #include "chrome/browser/ui/browser_finder.h" On 2012/07/24 23:41:55, msw wrote: > nit: is this include necessary? Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.cc:13: #include "chrome/browser/ui/tab_contents/tab_contents.h" On 2012/07/24 23:41:55, msw wrote: > nit: is this include necessary? Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.h (right): http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.h:11: class ActionBoxMenu; On 2012/07/24 23:41:55, msw wrote: > nit: remove this unused forward decl. Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.h:39: bool bookmark_state_; On 2012/07/24 23:41:55, msw wrote: > nit: explain the significance of bookmark_state_ here. Done.
Thanks for addressing so many nits! http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.cc:77: // A preference that tracks order of extensions in an toolbar when On 2012/07/25 21:09:21, yefimt wrote: > On 2012/07/24 23:41:55, msw wrote: > > grammar nits: "the order", "a toolbar", and "is enabled". > > Done. Almost, please change "an toolbar" to "a toolbar". http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.cc (right): http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:145: bool found_in_toolbar = FindInExtensionList(extension, toolbar_items_); On 2012/07/25 21:09:21, yefimt wrote: > On 2012/07/24 23:41:55, msw wrote: > > nit: Instead of using two bools here, use a single extensions::ExtensionList* > > |container| or |list| or similar that's NULL or points to the right container; > > that should simplify the code below (and the code here if you implement my > > suggestion in the header about returning the containing list). > > I believe it is much more readable as it is. Using a pointer will make portion > where extension is removed from one of the list more difficult to parse. I disagree, it'd be: RemoveExtension(extension, container); Which is shorter and seems clearer than the current patch: 160 if (found_in_toolbar) 161 RemoveExtension(extension, &toolbar_items_); 162 if (found_in_action_box_menu) 163 RemoveExtension(extension, &action_box_menu_items_); It's not a big deal though, I suppose. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:250: // to the action box. On 2012/07/25 21:09:21, yefimt wrote: > On 2012/07/24 23:41:55, msw wrote: > > nit: explain why the prefs list might miss some extensions. > > I dont think I can, it could be hundreds of reasons. For any of them we need to > add these extensions to the list or user would never see these extensions. > (Simplest reason preferences file was edited or corrupted). Fair enough, consider at least adding "The prefs list may omit some extensions." or similar. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:303: for (ExtensionList::const_iterator iter = sorted.begin(); On 2012/07/25 21:09:21, yefimt wrote: > On 2012/07/24 23:41:55, msw wrote: > > nit: use toolbar_items_.insert for |sorted| (like unsorted), instead of > looping. > > Some could be NULL and need to be excluded Good point! Sorry. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:337: const extensions::Extension* extension = On 2012/07/25 21:09:21, yefimt wrote: > On 2012/07/24 23:41:55, msw wrote: > > nit: declare a single non-const Extension* outside the loop and re-use here. > > GetExtensionById() returns const Extension*, so it wont work unless i do > const_cast Fair enough! http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/action_box_menu_model.h (right): http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:39: virtual void ExecuteCommand(int command_id) OVERRIDE; On 2012/07/25 21:09:21, yefimt wrote: > On 2012/07/24 23:41:55, msw wrote: > > nit: Match OVERRIDE declaration order and visibility (public); same for > Observe. > > Done - for order > As for visibility, aa@ asked me to make them private. I dont remember if it was > this place or not, but I'm sure he asked for all similar places. OK, there's no official style guide rule, so I won't press further, but I generally prefer to retain the original visibility. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.cc (right): http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:41: // We have icons, set this so we get the taller menu style. On 2012/07/25 21:09:21, yefimt wrote: > On 2012/07/24 23:41:55, msw wrote: > > nit: I prefer making statements about the objects without using "we", use > > impersonal verb conjugation if needed: > > http://en.wikipedia.org/wiki/Impersonal_verb > > (this general preference nit applies here, below, and elsewhere) > > Perhaps this particular comment isn't needed at all. > > Removed. > As I'm new to chrome, I often use existing code as a template. In this case see > wrench_menu.cc:762 :) Very true, I just prefer explicitly specifying the sentence subject/object for clarity. It's only a preference nit, but I think it's worthwhile habit. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:135: if (model_->GetTypeAt(model_index) == ui::MenuModel::TYPE_COMMAND) { On 2012/07/25 21:09:21, yefimt wrote: > On 2012/07/24 23:41:55, msw wrote: > > nit: Can you clarify generally what menu items are and aren't commands, and > > why/how they're styled and handled differently? > > I did it looking at how other menus are implemented. Currently here they all > command type. I guess if non command menu items will be added later they wont > break this code. Then I'd make this a DCHECK instead of a conditional. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:149: views::MenuItemView* ActionBoxMenu::AddBookmarkMenuItem( On 2012/07/25 21:09:21, yefimt wrote: > On 2012/07/24 23:41:55, msw wrote: > > nit: the return value is unused in the sole caller, consider removing it. > > Would it be better to have it consistent with all > MenuItemView::Add/AppendMenuItem*() functions? Sure, I suppose that's fine. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.h (right): http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:5: #ifndef CHROME_BROWSER_UI_VIEWS_ACTION_BOX_MENU_H_ On 2012/07/25 21:09:21, yefimt wrote: > On 2012/07/24 23:41:55, msw wrote: > > nit: do these files belong in the toolbar subdir? > > Not sure. wrench_menu.* is in this folder too. > I dont think toolbar is a right place, maybe location_bar? Ah, I made that recommendation too hastily, thinking that chrome/browser/ui/toolbar/action_box_menu_model.[h|cc] was actually in ui/views... Perhaps this does belong in location_bar? I think it's fine to leave it here for now. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:48: // BrowserActionView::Delegate and DragController overrides: On 2012/07/25 21:09:21, yefimt wrote: > On 2012/07/24 23:41:55, msw wrote: > > nit: Please differentiate OVERRIDEs by declaration, if possible. > > I was asked lump them together :) Okay, this is fine then. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.cc (right): http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:72: if (tab_id >= 0) { On 2012/07/25 21:09:21, yefimt wrote: > On 2012/07/24 23:41:55, msw wrote: > > nit: can this be merged with (or made more similar to) PaintChildren? Why use > > the icon width/height here, but the view width()/height() there, etc? > > Not sure, I dont know this code enough Okay, you didn't change the logic AFAICT, so it's fine to leave as-is. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:392: SetTooltipText(disable_tooltip_ ? string16() : GetTextForTooltip()); On 2012/07/24 23:41:55, msw wrote: > nit: shouldn't |disable_tooltip_| be checked in GetTextForTooltip instead? Ah, I may have just conflated the purpose of the related code here. Please rename GetTextForTooltip() to GetName(); I think that's clearer. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.h (right): http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:44: // Returns relative position of the |button_| inside BrowserActionView. On 2012/07/25 21:09:21, yefimt wrote: > On 2012/07/24 23:41:55, msw wrote: > > nit: use "button" as general terminology, as this is slightly disjoint from > the > > actual BrowserActionView implementation. > > Done, but i was asked to use specific variable names:) Doh! Sorry I missed that earlier comment; either way is fine. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.cc (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:31: // or -1 if not found. nit: comment fits on one line. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:32: int FindInExtensionList(const Extension* extension, Why return an index if the two callers just convert that to a bool? http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.h (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.h:118: // Fills |list| with extensions based on provided |order| nit: append a period at the end. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.h (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:83: // Set to true when bookmark icon need to show bookmarked state. nit: "the bookmark icon" and s/need/needs. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.h (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.h:39: // Set to true when current page is bookmarked. To passed to action box menu nit: "*the* current", "To *be* passed", and "*the* action box".
http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/action_box_menu_model.h (right): http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:39: virtual void ExecuteCommand(int command_id) OVERRIDE; On 2012/07/25 23:02:03, msw wrote: > On 2012/07/25 21:09:21, yefimt wrote: > > On 2012/07/24 23:41:55, msw wrote: > > > nit: Match OVERRIDE declaration order and visibility (public); same for > > Observe. > > > > Done - for order > > As for visibility, aa@ asked me to make them private. I dont remember if it > was > > this place or not, but I'm sure he asked for all similar places. > > OK, there's no official style guide rule, so I won't press further, but I > generally prefer to retain the original visibility. (FWIW, I didn't say anything on this patch to my knowledge, but I generally direct people to make OVERRIDE functions as private as possible without regard to their original visibility. I can see arguments both ways though.)
http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.h (right): http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:44: virtual void ExecuteCommand(int id) OVERRIDE; On 2012/07/25 21:09:21, yefimt wrote: > On 2012/07/24 23:41:55, msw wrote: > > nit: Match OVERRIDE declaration visibility (public), same for others below. > > As in other place I was asked by reviewer always make them private unless they > need to be called from outside. Yes, please, keep it private! outside of the public interface of ActionBoxMenu. this is just an implementation detail that should not be called directly by the clients. No one is going to do: ActionBoxMenu* menu = new ActionBoxMenu(...); menu->ExecuteCommand(..); This will be done through the delegate_->ExecuteCommand(...);
http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/action_box_menu_model.h (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:27: size_t action_box_extensions_size(); minor/tiny nit: Do they asked you to write in unix_hacker style for this function? We usually use this style for inlined functions, i.e, size_t count() const { return count_; } If not, may be rename this to GetExtensionsCount, or GetActionBoxExtensionsCount, or something...
http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.cc:1194: bool action_box_enabled = extensions::switch_utils::IsActionBoxEnabled(); Nit: Shorter: SetExtensionPrefFromVector(extensions::switch_utils::IsActionBoxEnabled() ? kExtensionActionBoxBar : kExtensionToolbar, extension_ids); http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.h:62: typedef std::vector<std::string> ExtensionIdSet; Nit: Not necessary in this change, but could you file a bug or something to change this typedef name to ExtensionIds (preferred) or ExtensionIdList? I ask because ExtensionIdSet sounds like a set, not a vector, and chrome/common/extensions/extension.h also defines an ExtensionIdSet type, which actually is a set, so this could lead to bugs. (If you want to change this in this CL since you're touching so many usages that's fine too.) http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.cc (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:32: int FindInExtensionList(const Extension* extension, On 2012/07/25 23:02:03, msw wrote: > Why return an index if the two callers just convert that to a bool? Yeah, this is weird. I think I agree with Mike that the best way to do this is instead to have a member function that looks through both |toolbar_items_| and |action_box_menu_items_| and returns the ExtensionList* which contains the extension in question, or NULL if neither. The only problem with this would be if an extension could be in both lists at once, but that's not supposed to happen, is it? http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:179: type == Nit: move this up onto prior line http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:180: chrome::NOTIFICATION_EXTENSION_BROWSER_ACTION_VISIBILITY_CHANGED) { Nit: Indent 4, not 6 http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:185: if (service_->extension_prefs()->GetBrowserActionVisibility(extension)) Nit: Combine with "} else {" above so you have "if ... else if ... else" http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:223: if (pos == end()) { Nit: No {} http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:269: if (!extension->browser_action()) Nit: Much shorter: if (extension->browser_action() && (std::find(toolbar_order.begin(), toolbar_order.end(), extension->id()) == toolbar_order.end()) && (std::find(action_box_order.begin(), action_box_order.end(), extension->id()) != action_box_order.end())) action_box_order.push_back(extension->id()); http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:323: // that are specified here. Nit: Even clearer: "It's possible for the extension order to contain items that aren't actually loaded on this machine. For example, when extension sync is on, we sync the extension order as-is but double-check with the user before syncing NPAPI-containing extensions, so if one of those is not actually synced, we'll get a NULL in the list. This sort of case can also happen if some error prevents an extension from loading." http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/action_box_menu_model.cc (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.cc:12: // that show before extensions. Nit: Thanks for adding this comment. Could you say more about the "menu IDs that show before extensions"? Do they start with 0? Where do they come from? http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/action_box_menu_model.h (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:20: class ActionBoxMenuModel : public ui::SimpleMenuModel, It seems a bit strange for this class to be both the menu model and the delegate. Based on the usage in ActionBoxMenu, shouldn't this class just be the menu model, and the ActionBoxMenu that owns it be the delegate? That way the overlapping overrides from MenuDelegate and SimpleMenuModel::Delegate can be implemented by as few functions as possible, all in one place. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:27: size_t action_box_extensions_size(); On 2012/07/26 02:43:40, tfarina wrote: > minor/tiny nit: Do they asked you to write in unix_hacker style for this > function? We usually use this style for inlined functions, i.e, size_t count() > const { return count_; } > > If not, may be rename this to GetExtensionsCount, or > GetActionBoxExtensionsCount, or something... The name here is correct, but the actual function definition should be inlined here as well instead of remaining in the .cc file. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:28: const extensions::Extension* GetActionBoxExtensionByIndex(int index); Nit: This function should also be renamed unix_hacker()-style and inlined. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:48: IdToEntensionIdMap id_to_extension_id_; Nit: Add "map_" to the end of the name http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.cc (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:50: // This menu is not deletable, Nit: Mike was wrong, this menu is in fact deletable; so just eliminate this first comment line and capitalize "Ignore" on the next line. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:132: if (model_->GetTypeAt(model_index) == ui::MenuModel::TYPE_COMMAND) { Nit: I agree with Mike that this should be a DCHECK until you actually need to handle other types. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:138: view->button()->SetShowMultipleIconStates(false); I reiterate: Why are these calls happening here instead of in BrowserActionView? Then that class wouldn't need to expose button(), dealing with ownership could be simpler, etc. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.h (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:84: bool bookmark_item_state_; Nit: For consistency with the rest of the codebase, I'd call this |starred_| and comment something like: "Set when the current page is bookmarked and thus the star icon should be drawn in the "starred" rather than "unstarred" state." http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.cc (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:53: button_ = new BrowserActionButton(extension, browser_, delegate_); Can move this into ViewHierarchyChanged() and remove the manual memory management if we move the accesses to button() from the caller to this class; see comments in ActionBoxMenu::PopulateMenu(). http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:90: button_->SetBounds(size.width(), size.height(), width(), Calling width() here isn't right, Layout() is responsible for determining the width. Plus the total width would include the view content offset and clearly the button itself shouldn't. In fact the whole layout of these objects at all is confusing. Is BrowserActionView supposed to contain a button plus the padding on the left and top? If so why is the padding included, why don't we just have the button directly, and then the owner of these views can lay them out with padding? And in that case, can't this class die entirely and we just have BrowserActionButton that extends the base button classes to do whatever additional special things it needs? http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:207: ShowContextMenuImpl(); Nit: Should you have a TODO about handling |source| and |point|, or not? What do these args mean? (2 places) http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:456: views::MenuRunner menu_runner(menu_model_adapter.CreateMenu()); MenuRunner objects should never be created on the stack; they should always be scoped_ptr<> members of the owning class, so that if the browser is torn down, the MenuRunner will get deleted correctly. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_actions_container.cc (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_actions_container.cc:458: TabContents* tab = chrome::GetActiveTabContents(browser_); Nit: Is it posisble to factor this out to some place where this code and BrowserActionsToolbarGtk (which has a copy of the same code) can share it? http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.h (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.h:41: bool bookmark_state_; Nit: As in action_box_menu.h, call this |starred_| (and rename the accessors as needed).
http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.cc:77: // A preference that tracks order of extensions in an toolbar when On 2012/07/25 23:02:03, msw wrote: > On 2012/07/25 21:09:21, yefimt wrote: > > On 2012/07/24 23:41:55, msw wrote: > > > grammar nits: "the order", "a toolbar", and "is enabled". > > > > Done. > > Almost, please change "an toolbar" to "a toolbar". Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.cc (right): http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:145: bool found_in_toolbar = FindInExtensionList(extension, toolbar_items_); On 2012/07/25 23:02:03, msw wrote: > On 2012/07/25 21:09:21, yefimt wrote: > > On 2012/07/24 23:41:55, msw wrote: > > > nit: Instead of using two bools here, use a single > extensions::ExtensionList* > > > |container| or |list| or similar that's NULL or points to the right > container; > > > that should simplify the code below (and the code here if you implement my > > > suggestion in the header about returning the containing list). > > > > I believe it is much more readable as it is. Using a pointer will make > portion > > where extension is removed from one of the list more difficult to parse. > > I disagree, it'd be: RemoveExtension(extension, container); > Which is shorter and seems clearer than the current patch: > > 160 if (found_in_toolbar) > 161 RemoveExtension(extension, &toolbar_items_); > 162 if (found_in_action_box_menu) > 163 RemoveExtension(extension, &action_box_menu_items_); > > It's not a big deal though, I suppose. Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:250: // to the action box. On 2012/07/25 23:02:03, msw wrote: > On 2012/07/25 21:09:21, yefimt wrote: > > On 2012/07/24 23:41:55, msw wrote: > > > nit: explain why the prefs list might miss some extensions. > > > > I dont think I can, it could be hundreds of reasons. For any of them we need > to > > add these extensions to the list or user would never see these extensions. > > (Simplest reason preferences file was edited or corrupted). > > Fair enough, consider at least adding "The prefs list may omit some extensions." > or similar. Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.cc (right): http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:135: if (model_->GetTypeAt(model_index) == ui::MenuModel::TYPE_COMMAND) { On 2012/07/25 23:02:03, msw wrote: > On 2012/07/25 21:09:21, yefimt wrote: > > On 2012/07/24 23:41:55, msw wrote: > > > nit: Can you clarify generally what menu items are and aren't commands, and > > > why/how they're styled and handled differently? > > > > I did it looking at how other menus are implemented. Currently here they all > > command type. I guess if non command menu items will be added later they wont > > break this code. > > Then I'd make this a DCHECK instead of a conditional. Done. http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.cc (right): http://codereview.chromium.org/10533086/diff/60023/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:392: SetTooltipText(disable_tooltip_ ? string16() : GetTextForTooltip()); On 2012/07/25 23:02:03, msw wrote: > On 2012/07/24 23:41:55, msw wrote: > > nit: shouldn't |disable_tooltip_| be checked in GetTextForTooltip instead? > > Ah, I may have just conflated the purpose of the related code here. > Please rename GetTextForTooltip() to GetName(); I think that's clearer. Done. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.cc (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.cc:1194: bool action_box_enabled = extensions::switch_utils::IsActionBoxEnabled(); On 2012/07/26 20:37:17, Peter Kasting wrote: > Nit: Shorter: > > SetExtensionPrefFromVector(extensions::switch_utils::IsActionBoxEnabled() ? > kExtensionActionBoxBar : kExtensionToolbar, extension_ids); Done. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.h:62: typedef std::vector<std::string> ExtensionIdSet; On 2012/07/26 20:37:17, Peter Kasting wrote: > Nit: Not necessary in this change, but could you file a bug or something to > change this typedef name to ExtensionIds (preferred) or ExtensionIdList? I ask > because ExtensionIdSet sounds like a set, not a vector, and > chrome/common/extensions/extension.h also defines an ExtensionIdSet type, which > actually is a set, so this could lead to bugs. > > (If you want to change this in this CL since you're touching so many usages > that's fine too.) Ok, I wrote it down to my task list and added TODO comment http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.cc (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:31: // or -1 if not found. On 2012/07/25 23:02:03, msw wrote: > nit: comment fits on one line. Done. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:32: int FindInExtensionList(const Extension* extension, On 2012/07/25 23:02:03, msw wrote: > Why return an index if the two callers just convert that to a bool? Done. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:32: int FindInExtensionList(const Extension* extension, On 2012/07/25 23:02:03, msw wrote: > Why return an index if the two callers just convert that to a bool? Done. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:179: type == On 2012/07/26 20:37:17, Peter Kasting wrote: > Nit: move this up onto prior line Done. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:180: chrome::NOTIFICATION_EXTENSION_BROWSER_ACTION_VISIBILITY_CHANGED) { On 2012/07/26 20:37:17, Peter Kasting wrote: > Nit: Indent 4, not 6 Done. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:185: if (service_->extension_prefs()->GetBrowserActionVisibility(extension)) On 2012/07/26 20:37:17, Peter Kasting wrote: > Nit: Combine with "} else {" above so you have "if ... else if ... else" I think current formatting is better because if statement fits to a single line. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:223: if (pos == end()) { On 2012/07/26 20:37:17, Peter Kasting wrote: > Nit: No {} Done. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:269: if (!extension->browser_action()) On 2012/07/26 20:37:17, Peter Kasting wrote: > Nit: Much shorter: > > if (extension->browser_action() && > (std::find(toolbar_order.begin(), toolbar_order.end(), extension->id()) == > toolbar_order.end()) && > (std::find(action_box_order.begin(), action_box_order.end(), > extension->id()) != action_box_order.end())) > action_box_order.push_back(extension->id()); Wow, it is shorter, but would it be shorter to collapse all white space and end of lines. Shorter is not always more readable. Sorry. I think current formatting is way more readable and consistent (last one is to Mike's comment to reverse condition in the last if statement). There are three conditions we use to filter extensions out. They are separately stated and easily parsable. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:323: // that are specified here. On 2012/07/26 20:37:17, Peter Kasting wrote: > Nit: Even clearer: > > "It's possible for the extension order to contain items that aren't actually > loaded on this machine. For example, when extension sync is on, we sync the > extension order as-is but double-check with the user before syncing > NPAPI-containing extensions, so if one of those is not actually synced, we'll > get a NULL in the list. This sort of case can also happen if some error > prevents an extension from loading." Done. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.h (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.h:118: // Fills |list| with extensions based on provided |order| On 2012/07/25 23:02:03, msw wrote: > nit: append a period at the end. Done. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/action_box_menu_model.cc (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.cc:12: // that show before extensions. On 2012/07/26 20:37:17, Peter Kasting wrote: > Nit: Thanks for adding this comment. Could you say more about the "menu IDs > that show before extensions"? Do they start with 0? Where do they come from? Done. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/action_box_menu_model.h (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:20: class ActionBoxMenuModel : public ui::SimpleMenuModel, On 2012/07/26 20:37:17, Peter Kasting wrote: > It seems a bit strange for this class to be both the menu model and the > delegate. Based on the usage in ActionBoxMenu, shouldn't this class just be the > menu model, and the ActionBoxMenu that owns it be the delegate? That way the > overlapping overrides from MenuDelegate and SimpleMenuModel::Delegate can be > implemented by as few functions as possible, all in one place. I'll look into it, dont have answer yet. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:27: size_t action_box_extensions_size(); On 2012/07/26 02:43:40, tfarina wrote: > minor/tiny nit: Do they asked you to write in unix_hacker style for this > function? We usually use this style for inlined functions, i.e, size_t count() > const { return count_; } > > If not, may be rename this to GetExtensionsCount, or > GetActionBoxExtensionsCount, or something... Done. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:28: const extensions::Extension* GetActionBoxExtensionByIndex(int index); On 2012/07/26 20:37:17, Peter Kasting wrote: > Nit: This function should also be renamed unix_hacker()-style and inlined. Done. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:48: IdToEntensionIdMap id_to_extension_id_; On 2012/07/26 20:37:17, Peter Kasting wrote: > Nit: Add "map_" to the end of the name Done. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.cc (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:50: // This menu is not deletable, On 2012/07/26 20:37:17, Peter Kasting wrote: > Nit: Mike was wrong, this menu is in fact deletable; so just eliminate this > first comment line and capitalize "Ignore" on the next line. Done. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:132: if (model_->GetTypeAt(model_index) == ui::MenuModel::TYPE_COMMAND) { On 2012/07/26 20:37:17, Peter Kasting wrote: > Nit: I agree with Mike that this should be a DCHECK until you actually need to > handle other types. Done. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:138: view->button()->SetShowMultipleIconStates(false); On 2012/07/26 20:37:17, Peter Kasting wrote: > I reiterate: Why are these calls happening here instead of in BrowserActionView? > Then that class wouldn't need to expose button(), dealing with ownership could > be simpler, etc. Done. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.h (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:83: // Set to true when bookmark icon need to show bookmarked state. On 2012/07/25 23:02:03, msw wrote: > nit: "the bookmark icon" and s/need/needs. Done. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.h:84: bool bookmark_item_state_; On 2012/07/26 20:37:17, Peter Kasting wrote: > Nit: For consistency with the rest of the codebase, I'd call this |starred_| and > comment something like: "Set when the current page is bookmarked and thus the > star icon should be drawn in the "starred" rather than "unstarred" state." Done. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.cc (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:53: button_ = new BrowserActionButton(extension, browser_, delegate_); On 2012/07/26 20:37:17, Peter Kasting wrote: > Can move this into ViewHierarchyChanged() and remove the manual memory > management if we move the accesses to button() from the caller to this class; > see comments in ActionBoxMenu::PopulateMenu(). Done. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:90: button_->SetBounds(size.width(), size.height(), width(), On 2012/07/26 20:37:17, Peter Kasting wrote: > Calling width() here isn't right, Layout() is responsible for determining the > width. Plus the total width would include the view content offset and clearly > the button itself shouldn't. > > In fact the whole layout of these objects at all is confusing. Is > BrowserActionView supposed to contain a button plus the padding on the left and > top? If so why is the padding included, why don't we just have the button > directly, and then the owner of these views can lay them out with padding? And > in that case, can't this class die entirely and we just have BrowserActionButton > that extends the base button classes to do whatever additional special things it > needs? I think you need to ask someone who wrote this code. You may get a meaningful reply in this case. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:207: ShowContextMenuImpl(); On 2012/07/26 20:37:17, Peter Kasting wrote: > Nit: Should you have a TODO about handling |source| and |point|, or not? What > do these args mean? (2 places) I dont know, it is an existing implementation and I'm trying to focus on a functionality I add, not on fixing code I dont understand well enough. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:456: views::MenuRunner menu_runner(menu_model_adapter.CreateMenu()); On 2012/07/26 20:37:17, Peter Kasting wrote: > MenuRunner objects should never be created on the stack; they should always be > scoped_ptr<> members of the owning class, so that if the browser is torn down, > the MenuRunner will get deleted correctly. Done. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_actions_container.cc (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_actions_container.cc:458: TabContents* tab = chrome::GetActiveTabContents(browser_); On 2012/07/26 20:37:17, Peter Kasting wrote: > Nit: Is it posisble to factor this out to some place where this code and > BrowserActionsToolbarGtk (which has a copy of the same code) can share it? Not sure, I dont know where I can put this code http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.h (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.h:39: // Set to true when current page is bookmarked. To passed to action box menu On 2012/07/25 23:02:03, msw wrote: > nit: "*the* current", "To *be* passed", and "*the* action box". Done. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.h:41: bool bookmark_state_; On 2012/07/26 20:37:17, Peter Kasting wrote: > Nit: As in action_box_menu.h, call this |starred_| (and rename the accessors as > needed). Done.
http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/action_box_menu_model.h (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:20: class ActionBoxMenuModel : public ui::SimpleMenuModel, On 2012/07/26 20:37:17, Peter Kasting wrote: > It seems a bit strange for this class to be both the menu model and the > delegate. Based on the usage in ActionBoxMenu, shouldn't this class just be the > menu model, and the ActionBoxMenu that owns it be the delegate? That way the > overlapping overrides from MenuDelegate and SimpleMenuModel::Delegate can be > implemented by as few functions as possible, all in one place. Done.
http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.cc (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:269: if (!extension->browser_action()) On 2012/07/31 00:10:11, yefimt wrote: > On 2012/07/26 20:37:17, Peter Kasting wrote: > > Nit: Much shorter: > > > > if (extension->browser_action() && > > (std::find(toolbar_order.begin(), toolbar_order.end(), extension->id()) > == > > toolbar_order.end()) && > > (std::find(action_box_order.begin(), action_box_order.end(), > > extension->id()) != action_box_order.end())) > > action_box_order.push_back(extension->id()); > > Wow, it is shorter, but would it be shorter to collapse all white space and end > of lines. Shorter is not always more readable. Sorry. > I think current formatting is way more readable and consistent (last one is to > Mike's comment to reverse condition in the last if statement). > There are three conditions we use to filter extensions out. They are separately > stated and easily parsable. Can you at least get rid of your temporaries? They each require a full line of text just to redeclare the iterator type. I don't really agree that separating them from each other -- or using continue for a common case -- is necessarily the most readable way to do things, but this is so trivial there's no point arguing over it. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.cc (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:90: button_->SetBounds(size.width(), size.height(), width(), On 2012/07/31 00:10:11, yefimt wrote: > On 2012/07/26 20:37:17, Peter Kasting wrote: > > Calling width() here isn't right, Layout() is responsible for determining the > > width. Plus the total width would include the view content offset and clearly > > the button itself shouldn't. > > > > In fact the whole layout of these objects at all is confusing. Is > > BrowserActionView supposed to contain a button plus the padding on the left > and > > top? If so why is the padding included, why don't we just have the button > > directly, and then the owner of these views can lay them out with padding? > And > > in that case, can't this class die entirely and we just have > BrowserActionButton > > that extends the base button classes to do whatever additional special things > it > > needs? > > I think you need to ask someone who wrote this code. You may get a meaningful > reply in this case. You at least need to use width() - size.width() for the button width. I think if you do that this function is probably OK. As for the larger question, if you're not in a position to answer, can you file a bug against the appropriate person? It seems a little unusual for you to be doing implementation work for the action box menu but not know what's up with the design of the action box menu, but as long as some relevant party gets the question I'm satisfied. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:207: ShowContextMenuImpl(); On 2012/07/31 00:10:11, yefimt wrote: > On 2012/07/26 20:37:17, Peter Kasting wrote: > > Nit: Should you have a TODO about handling |source| and |point|, or not? What > > do these args mean? (2 places) > > I dont know, it is an existing implementation and I'm trying to focus on a > functionality I add, not on fixing code I dont understand well enough. But you're adding one of the calls to this function. How can you not understand at least the code you're adding? If you don't understand the callsite here and need to file a bug against the relevant person to investigate (see comment elsewhere), that's arguably OK, but you need to at least know what the callsite you're adding elsewhere in this file does.
http://codereview.chromium.org/10533086/diff/73002/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.h (right): http://codereview.chromium.org/10533086/diff/73002/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:50: virtual gfx::Size GetViewContentOffset() const = 0; This should return a gfx::Point, right? It's an offset, not a size.
http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.cc (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:269: if (!extension->browser_action()) On 2012/07/31 02:30:42, Peter Kasting wrote: > On 2012/07/31 00:10:11, yefimt wrote: > > On 2012/07/26 20:37:17, Peter Kasting wrote: > > > Nit: Much shorter: > > > > > > if (extension->browser_action() && > > > (std::find(toolbar_order.begin(), toolbar_order.end(), > extension->id()) > > == > > > toolbar_order.end()) && > > > (std::find(action_box_order.begin(), action_box_order.end(), > > > extension->id()) != action_box_order.end())) > > > action_box_order.push_back(extension->id()); > > > > Wow, it is shorter, but would it be shorter to collapse all white space and > end > > of lines. Shorter is not always more readable. Sorry. > > I think current formatting is way more readable and consistent (last one is to > > Mike's comment to reverse condition in the last if statement). > > There are three conditions we use to filter extensions out. They are > separately > > stated and easily parsable. > > Can you at least get rid of your temporaries? They each require a full line of > text just to redeclare the iterator type. > > I don't really agree that separating them from each other -- or using continue > for a common case -- is necessarily the most readable way to do things, but this > is so trivial there's no point arguing over it. Done. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.cc (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:90: button_->SetBounds(size.width(), size.height(), width(), On 2012/07/31 02:30:42, Peter Kasting wrote: > On 2012/07/31 00:10:11, yefimt wrote: > > On 2012/07/26 20:37:17, Peter Kasting wrote: > > > Calling width() here isn't right, Layout() is responsible for determining > the > > > width. Plus the total width would include the view content offset and > clearly > > > the button itself shouldn't. > > > > > > In fact the whole layout of these objects at all is confusing. Is > > > BrowserActionView supposed to contain a button plus the padding on the left > > and > > > top? If so why is the padding included, why don't we just have the button > > > directly, and then the owner of these views can lay them out with padding? > > And > > > in that case, can't this class die entirely and we just have > > BrowserActionButton > > > that extends the base button classes to do whatever additional special > things > > it > > > needs? > > > > I think you need to ask someone who wrote this code. You may get a meaningful > > reply in this case. > > You at least need to use width() - size.width() for the button width. I think > if you do that this function is probably OK. > > As for the larger question, if you're not in a position to answer, can you file > a bug against the appropriate person? It seems a little unusual for you to be > doing implementation work for the action box menu but not know what's up with > the design of the action box menu, but as long as some relevant party gets the > question I'm satisfied. It has nothing to do with architecture. It was an existing class BrowserActionView used in toolbar. I'm using this class in Action Box menu. I dont need to know BrowserActionView implementation details for that. .h file is enough in most cases. Yes I had to do some minor changes in this class, in this specific case line in question (line 90) had a hardcoded offset, so I added a way to get an offset from a parent class. As for width() I agree it should be width() - size.width(), fixed. http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:207: ShowContextMenuImpl(); On 2012/07/31 02:30:42, Peter Kasting wrote: > On 2012/07/31 00:10:11, yefimt wrote: > > On 2012/07/26 20:37:17, Peter Kasting wrote: > > > Nit: Should you have a TODO about handling |source| and |point|, or not? > What > > > do these args mean? (2 places) > > > > I dont know, it is an existing implementation and I'm trying to focus on a > > functionality I add, not on fixing code I dont understand well enough. > > But you're adding one of the calls to this function. How can you not understand > at least the code you're adding? > > If you don't understand the callsite here and need to file a bug against the > relevant person to investigate (see comment elsewhere), that's arguably OK, but > you need to at least know what the callsite you're adding elsewhere in this file > does. :) As you noted earlier it were two functions with identical bodies, both didn't use function's parameters. So per your request I moved body to a third function. I doesn't require much understanding consider that both functions not related to action box directly. http://codereview.chromium.org/10533086/diff/73002/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.h (right): http://codereview.chromium.org/10533086/diff/73002/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:50: virtual gfx::Size GetViewContentOffset() const = 0; On 2012/07/31 15:59:14, Matt Perry wrote: > This should return a gfx::Point, right? It's an offset, not a size. Done.
http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.cc (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:207: ShowContextMenuImpl(); On 2012/07/31 17:19:35, yefimt wrote: > On 2012/07/31 02:30:42, Peter Kasting wrote: > > On 2012/07/31 00:10:11, yefimt wrote: > > > On 2012/07/26 20:37:17, Peter Kasting wrote: > > > > Nit: Should you have a TODO about handling |source| and |point|, or not? > > What > > > > do these args mean? (2 places) > > > > > > I dont know, it is an existing implementation and I'm trying to focus on a > > > functionality I add, not on fixing code I dont understand well enough. > > > > But you're adding one of the calls to this function. How can you not > understand > > at least the code you're adding? > > > > If you don't understand the callsite here and need to file a bug against the > > relevant person to investigate (see comment elsewhere), that's arguably OK, > but > > you need to at least know what the callsite you're adding elsewhere in this > file > > does. > > :) > As you noted earlier it were two functions with identical bodies, both didn't > use function's parameters. So per your request I moved body to a third function. > I doesn't require much understanding consider that both functions not related to > action box directly. I'm talking about BrowserActionButton::ShowContextMenu(), which is far as I can tell is brand new in your change.
http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.cc (right): http://codereview.chromium.org/10533086/diff/67001/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:207: ShowContextMenuImpl(); On 2012/07/31 18:29:30, Peter Kasting wrote: > On 2012/07/31 17:19:35, yefimt wrote: > > On 2012/07/31 02:30:42, Peter Kasting wrote: > > > On 2012/07/31 00:10:11, yefimt wrote: > > > > On 2012/07/26 20:37:17, Peter Kasting wrote: > > > > > Nit: Should you have a TODO about handling |source| and |point|, or not? > > > > What > > > > > do these args mean? (2 places) > > > > > > > > I dont know, it is an existing implementation and I'm trying to focus on a > > > > functionality I add, not on fixing code I dont understand well enough. > > > > > > But you're adding one of the calls to this function. How can you not > > understand > > > at least the code you're adding? > > > > > > If you don't understand the callsite here and need to file a bug against the > > > relevant person to investigate (see comment elsewhere), that's arguably OK, > > but > > > you need to at least know what the callsite you're adding elsewhere in this > > file > > > does. > > > > :) > > As you noted earlier it were two functions with identical bodies, both didn't > > use function's parameters. So per your request I moved body to a third > function. > > I doesn't require much understanding consider that both functions not related > to > > action box directly. > > I'm talking about BrowserActionButton::ShowContextMenu(), which is far as I can > tell is brand new in your change. It is so strange. Somehow codereview is confused. If you look at original file (not in codereview), you will see BrowserActionButton::ShowContextMenu() is in the same place and it was one of the functions you asked me to fix before.
Only a couple of non-nits left. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/10533086/diff/78003/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.h:62: // TODO(yefim): rename to ExtensionIdList. Nit: Or ExtensionIds, which might be less misleading as this isn't a list. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.cc (right): http://codereview.chromium.org/10533086/diff/78003/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:179: if (service_->extension_prefs()->GetBrowserActionVisibility(extension)) Nit: Combine with else just above http://codereview.chromium.org/10533086/diff/78003/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:234: if (IsInExtensionList(extension, action_box_menu_items_)) Nit: Shorter: return IsInExtensionList(extension, action_box_menu_items_) ? &action_box_menu_items_ : NULL; http://codereview.chromium.org/10533086/diff/78003/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.h (right): http://codereview.chromium.org/10533086/diff/78003/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.h:88: const extensions::Extension* toolbar_item_by_index(int index) const { Nit: Using size_t for these vector-accessing functions would be more appropriate. (The only caller of this is already unsigned anyway.) http://codereview.chromium.org/10533086/diff/78003/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.h:97: size_t action_box_extensions_size() { Nit: You can replace both of these with: const extensions::ExtensionList& action_box_menu_items() const { return action_box_menu_items_; } This will allow some simplification elsewhere. See comments in ActionBoxMenuModel and ActionBoxMenu. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/action_box_menu_model.cc (right): http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.cc:23: for (size_t i = 0; i < action_box_extensions_size(); ++i) { Nit: If the extension toolbar model provides an action_box_menu_items() accessor, you can call that here and then simply iterate over it. This eliminates the need for action_box_extensions_size() and is part 1 of 2 in eliminating GetActionBoxExtensionByIndex(). http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/action_box_menu_model.h (right): http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:25: size_t action_box_extensions_size() { Nit: These can both be eliminated, see comments in the .cc file and ActionBoxMenu. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:41: ExtensionService* extension_service_; Nit: This can be eliminated. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.cc (right): http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:44: menu_runner_.reset(new views::MenuRunner(root_)); Do this in RunMenu() rather than here; this is common convention and also makes calling RunMenu() multiple times safe (not that you actually do this today). http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:141: const extensions::Extension* extension = Nit: If the extension toolbar model provides an action_box_menu_items() accessor, this can be changed to: const extensions::ExtensionList& menu_items = extensions::ExtensionSystem::Get(browser_->profile())-> extension_service()->toolbar_model()->action_box_menu_items(); for (...) { ... BrowserActionView* view = new BrowserActionView(menu_items[index], ... This is part 2 of 2 in eliminating ActionBoxMenuModel::GetActionBoxExtensionByIndex(), and avoids plumbing this sort of data through an extra class rather than getting it directly from here. (Direct dependencies like this make refactoring the intermediate classes easier later by avoiding unnecessary plumbing.) http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.cc (right): http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:62: SkBitmap icon = button_->extension()->browser_action()->GetIcon(tab_id); Nit: We don't protect this call with a >= 0 check, but we do protect PaintBadge (both places it's called). We should be consistent. My reading of the definitions is that protection is unnecessary. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:63: if (icon.isNull()) Nit: Can this happen? It doesn't seem like it from reading the callee. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:432: int tab_id = delegate_->GetCurrentTabId(); Nit: This seems like another case where just calling GetTitle() unilaterally ought to be fine, even if the tab ID is negative. (Are we even going to reach here with a negative tab ID?) At that point I would just inline the remaining bit of code here into the lone caller. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:459: menu_runner_.reset(); Don't do this, it is explicitly wrong because |this| is already deleted at this point. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.h (right): http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:37: // Need DragControler here because BrowserActionView could be dragged/dropped. Nit: DragControler -> DragController http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:52: virtual bool NeedToShowMultipleIconStates() const { return true; } Nit: Never define virtuals inline. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:56: Delegate() {} Nit: Omit this constructor for interface classes -- the pure virtual methods already guarantee no one can instantiate this directly. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:163: virtual void ShowContextMenu(const gfx::Point& p, I just double-checked the file in svn here ( http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/views/brows... ) and it really doesn't have this function. I also think we don't need this override. The default implementation in view.cc already calls over to ShowContextMenuForView(), which should work fine. So I think you can just omit this function entirely, and eliminate ShowContextMenuImpl() by inlining its contents in ShowContextMenuForView(). http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/ex... File chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.h (right): http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/ex... chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.h:93: // Browser associated with BrowserActionOverflowMenuController. Nit: This comment doesn't add anything as-is (in fact it's kind of confusing considering BrowserActionOverflowMenuController is the name of this class). http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.h (right): http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.h:24: void set_starred(bool starred) { Nit: We often put the whole definition on one line for these short inlined accessors.
http://codereview.chromium.org/10533086/diff/78003/chrome/browser/extensions/... File chrome/browser/extensions/extension_prefs.h (right): http://codereview.chromium.org/10533086/diff/78003/chrome/browser/extensions/... chrome/browser/extensions/extension_prefs.h:62: // TODO(yefim): rename to ExtensionIdList. On 2012/08/03 23:17:42, Peter Kasting wrote: > Nit: Or ExtensionIds, which might be less misleading as this isn't a list. Done. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.cc (right): http://codereview.chromium.org/10533086/diff/78003/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:179: if (service_->extension_prefs()->GetBrowserActionVisibility(extension)) On 2012/08/03 23:17:42, Peter Kasting wrote: > Nit: Combine with else just above Done. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.cc:234: if (IsInExtensionList(extension, action_box_menu_items_)) On 2012/08/03 23:17:42, Peter Kasting wrote: > Nit: Shorter: > > return IsInExtensionList(extension, action_box_menu_items_) ? > &action_box_menu_items_ : NULL; Done. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/extensions/... File chrome/browser/extensions/extension_toolbar_model.h (right): http://codereview.chromium.org/10533086/diff/78003/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.h:88: const extensions::Extension* toolbar_item_by_index(int index) const { On 2012/08/03 23:17:42, Peter Kasting wrote: > Nit: Using size_t for these vector-accessing functions would be more > appropriate. (The only caller of this is already unsigned anyway.) Removed http://codereview.chromium.org/10533086/diff/78003/chrome/browser/extensions/... chrome/browser/extensions/extension_toolbar_model.h:97: size_t action_box_extensions_size() { On 2012/08/03 23:17:42, Peter Kasting wrote: > Nit: You can replace both of these with: > > const extensions::ExtensionList& action_box_menu_items() const { > return action_box_menu_items_; > } > > This will allow some simplification elsewhere. See comments in > ActionBoxMenuModel and ActionBoxMenu. Done. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/action_box_menu_model.cc (right): http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.cc:23: for (size_t i = 0; i < action_box_extensions_size(); ++i) { On 2012/08/03 23:17:42, Peter Kasting wrote: > Nit: If the extension toolbar model provides an action_box_menu_items() > accessor, you can call that here and then simply iterate over it. This > eliminates the need for action_box_extensions_size() and is part 1 of 2 in > eliminating GetActionBoxExtensionByIndex(). I kind of did it, but I think ActionBoxMenuModel should be the only model exposed to ActionBoxMenu. So it provides own action_box_menu_items() http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/action_box_menu_model.h (right): http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:25: size_t action_box_extensions_size() { On 2012/08/03 23:17:42, Peter Kasting wrote: > Nit: These can both be eliminated, see comments in the .cc file and > ActionBoxMenu. Done. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/action_box_menu_model.h:41: ExtensionService* extension_service_; On 2012/08/03 23:17:42, Peter Kasting wrote: > Nit: This can be eliminated. I still need to get to toolbar model, so I use extension service. Very likely it will be needed when menu actions are implemented too. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.cc (right): http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:44: menu_runner_.reset(new views::MenuRunner(root_)); On 2012/08/03 23:17:42, Peter Kasting wrote: > Do this in RunMenu() rather than here; this is common convention and also makes > calling RunMenu() multiple times safe (not that you actually do this today). Done. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:141: const extensions::Extension* extension = On 2012/08/03 23:17:42, Peter Kasting wrote: > Nit: If the extension toolbar model provides an action_box_menu_items() > accessor, this can be changed to: > > const extensions::ExtensionList& menu_items = > extensions::ExtensionSystem::Get(browser_->profile())-> > extension_service()->toolbar_model()->action_box_menu_items(); > for (...) { > ... > BrowserActionView* view = new BrowserActionView(menu_items[index], ... > > This is part 2 of 2 in eliminating > ActionBoxMenuModel::GetActionBoxExtensionByIndex(), and avoids plumbing this > sort of data through an extra class rather than getting it directly from here. > (Direct dependencies like this make refactoring the intermediate classes easier > later by avoiding unnecessary plumbing.) Would it be logical to get |menu_items| from |model_|? http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.cc (right): http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:62: SkBitmap icon = button_->extension()->browser_action()->GetIcon(tab_id); On 2012/08/03 23:17:42, Peter Kasting wrote: > Nit: We don't protect this call with a >= 0 check, but we do protect PaintBadge > (both places it's called). We should be consistent. My reading of the > definitions is that protection is unnecessary. This is another case when asking people who did most of changes on this file would be better :) I see that chrome::GetActiveTabContents() could return NULL and in this case tab_id would be -1. The reason they dont check for GEtIcon is because it will return default icon if it doesnt exist. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:63: if (icon.isNull()) On 2012/08/03 23:17:42, Peter Kasting wrote: > Nit: Can this happen? It doesn't seem like it from reading the callee. Code in the trunk was changed by now, and these two lines are gone. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:432: int tab_id = delegate_->GetCurrentTabId(); On 2012/08/03 23:17:42, Peter Kasting wrote: > Nit: This seems like another case where just calling GetTitle() unilaterally > ought to be fine, even if the tab ID is negative. (Are we even going to reach > here with a negative tab ID?) > > At that point I would just inline the remaining bit of code here into the lone > caller. Done. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:459: menu_runner_.reset(); On 2012/08/03 23:17:42, Peter Kasting wrote: > Don't do this, it is explicitly wrong because |this| is already deleted at this > point. Done. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.h (right): http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:37: // Need DragControler here because BrowserActionView could be dragged/dropped. On 2012/08/03 23:17:42, Peter Kasting wrote: > Nit: DragControler -> DragController Done. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:52: virtual bool NeedToShowMultipleIconStates() const { return true; } On 2012/08/03 23:17:42, Peter Kasting wrote: > Nit: Never define virtuals inline. Done. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:56: Delegate() {} On 2012/08/03 23:17:42, Peter Kasting wrote: > Nit: Omit this constructor for interface classes -- the pure virtual methods > already guarantee no one can instantiate this directly. Done. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.h:163: virtual void ShowContextMenu(const gfx::Point& p, On 2012/08/03 23:17:42, Peter Kasting wrote: > I just double-checked the file in svn here ( > http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/views/brows... > ) and it really doesn't have this function. > > I also think we don't need this override. The default implementation in view.cc > already calls over to ShowContextMenuForView(), which should work fine. So I > think you can just omit this function entirely, and eliminate > ShowContextMenuImpl() by inlining its contents in ShowContextMenuForView(). You are right it was removed in Revision 147178. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/ex... File chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.h (right): http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/ex... chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.h:93: // Browser associated with BrowserActionOverflowMenuController. On 2012/08/03 23:17:42, Peter Kasting wrote: > Nit: This comment doesn't add anything as-is (in fact it's kind of confusing > considering BrowserActionOverflowMenuController is the name of this class). Done. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/lo... File chrome/browser/ui/views/location_bar/action_box_button_view.h (right): http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/lo... chrome/browser/ui/views/location_bar/action_box_button_view.h:24: void set_starred(bool starred) { On 2012/08/03 23:17:42, Peter Kasting wrote: > Nit: We often put the whole definition on one line for these short inlined > accessors. Done.
LGTM (didn't do a full re-review) http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/ac... File chrome/browser/ui/views/action_box_menu.cc (right): http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/ac... chrome/browser/ui/views/action_box_menu.cc:141: const extensions::Extension* extension = On 2012/08/07 00:47:06, yefimt wrote: > On 2012/08/03 23:17:42, Peter Kasting wrote: > > Nit: If the extension toolbar model provides an action_box_menu_items() > > accessor, this can be changed to: > > > > const extensions::ExtensionList& menu_items = > > extensions::ExtensionSystem::Get(browser_->profile())-> > > extension_service()->toolbar_model()->action_box_menu_items(); > > for (...) { > > ... > > BrowserActionView* view = new BrowserActionView(menu_items[index], ... > > > > This is part 2 of 2 in eliminating > > ActionBoxMenuModel::GetActionBoxExtensionByIndex(), and avoids plumbing this > > sort of data through an extra class rather than getting it directly from here. > > > (Direct dependencies like this make refactoring the intermediate classes > easier > > later by avoiding unnecessary plumbing.) > > Would it be logical to get |menu_items| from |model_|? As you've done in patch set 20? I don't think it provides any advantage -- after all, the extension service is already accessible directly from here without plumbing any new variables (see code in previous comment set) -- and it requires exposing another method on the menu model, enlarging the API surface. So I view it as a slightly worse solution. But if you really feel strongly that it's superior, I won't stop you. http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.cc (right): http://codereview.chromium.org/10533086/diff/78003/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:62: SkBitmap icon = button_->extension()->browser_action()->GetIcon(tab_id); On 2012/08/07 00:47:06, yefimt wrote: > On 2012/08/03 23:17:42, Peter Kasting wrote: > > Nit: We don't protect this call with a >= 0 check, but we do protect > PaintBadge > > (both places it's called). We should be consistent. My reading of the > > definitions is that protection is unnecessary. > > This is another case when asking people who did most of changes on this file > would be better :) > I see that chrome::GetActiveTabContents() could return NULL and in this case > tab_id would be -1. > The reason they dont check for GEtIcon is because it will return default icon if > it doesnt exist. Right -- I'm saying that PaintBadge() should also do the right thing if the tab_id is negative, since it will call GetBadgeText() first and that will return the empty string. So we can just remove the conditionals that guard the two PaintBadge() calls and call them all the time. http://codereview.chromium.org/10533086/diff/71007/chrome/browser/ui/views/br... File chrome/browser/ui/views/browser_action_view.cc (right): http://codereview.chromium.org/10533086/diff/71007/chrome/browser/ui/views/br... chrome/browser/ui/views/browser_action_view.cc:69: SkBitmap icon = *button_->extension()->browser_action()->GetIcon( Nit: Break after '=' instead
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yefim@chromium.org/10533086/71009
Change committed as 150869 |
