Extension Toolbar redesign, part 1 (overflow)

chrome/browser/ui/views/toolbar/browser_actions_container.h

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_UI_VIEWS_TOOLBAR_BROWSER_ACTIONS_CONTAINER_H_
 #define CHROME_BROWSER_UI_VIEWS_TOOLBAR_BROWSER_ACTIONS_CONTAINER_H_
 
 #include "base/observer_list.h"
 #include "chrome/browser/extensions/extension_keybinding_registry.h"
 #include "chrome/browser/extensions/extension_toolbar_model.h"
@@ skipping 24 matching lines @@
 }
 
 namespace gfx {
 class SlideAnimation;
 }
 
 namespace views {
 class ResizeArea;
 }
 
-////////////////////////////////////////////////////////////////////////////////
+// The BrowserActionsContainer is a container view, responsible for drawing the
+// browser action icons (extensions that add icons to the toolbar). It comes in
+// two flavors, a main container (when residing on the toolbar) and an overflow
+// container (that resides in the main application menu, known as the
+// hot dog/hamburger menu).

[Peter Kasting, 2014/06/30 23:36:34]

Nit: Again, do not use "hot dog" or "hamburger" to refer to the Chrome menu in
code/comments.  (multiple places)

[Finnur, 2014/07/02 16:59:56]

Done.

 //
-// The BrowserActionsContainer is a container view, responsible for drawing the
-// browser action icons (extensions that add icons to the toolbar).
+// When in 'main' mode, the container supports the full functionality of a
+// BrowserActionContainer but in 'overflow' mode the container is effectively

[Peter Kasting, 2014/06/30 23:36:33]

Nit: BrowserActionContainer but -> BrowserActionContainer, but

[Finnur, 2014/07/02 16:59:56]

Done.

+// just an overflow for the 'main' toolbar (shows only the icons that the main
+// toolbar does not) and as such does not have an overflow itself. The overflow
+// container also does not support resizing. Since the main container only shows
+// icons in the Chrome toolbar, it is limited to a single row of icons. The
+// overflow container, however, is allowed to display icons in multiple rows.
 //
-// The container is placed flush against the omnibox and wrench menu, and its
-// layout looks like:
+// The main container is placed flush against the omnibox and hot dog menu,
+// whereas the overflow container is placed within the hot dog menu. The
+// layout is similar to this:
 //   rI_I_IcCs
 // Where the letters are as follows:
 //   r: An invisible resize area.  This is ToolbarView::kStandardSpacing pixels
-//      wide and directly adjacent to the omnibox.
+//      wide and directly adjacent to the omnibox. Only shown for the main
+//      container.
 //   I: An icon.  This is as wide as the IDR_BROWSER_ACTION image.
 //   _: kItemSpacing pixels of empty space.
 //   c: kChevronSpacing pixels of empty space.  Only present if C is present.
-//   C: An optional chevron, visible for overflow.  As wide as the
-//      IDR_BROWSER_ACTIONS_OVERFLOW image.
+//   C: An optional chevron, as wide as the IDR_BROWSER_ACTIONS_OVERFLOW image,
+//      and visible only when all of the following statements are true:
+//      - This is the main container.

[Peter Kasting, 2014/06/30 23:36:34]

Nit: This point is already clear from the fact that this whole description is
about the main container.  Remove.

[Finnur, 2014/07/02 16:59:57]

Done.

+//      - The container is set to a width smaller than needed to show all icons.
+//      - There is no other container in 'overflow' mode to handle the
+//        non-visible icons for this container.
+//      NOTE: When the BrowserActionsContainer is in 'overflow mode', the icons
+//            for the 'main' container overflow to a special overflow container
+//            inside the wrench menu.

[Peter Kasting, 2014/06/30 23:36:33]

Nit: This NOTE seems like a rehash of comments above, I'd remove it.

[Finnur, 2014/07/02 16:59:57]

Done.

 //   s: ToolbarView::kStandardSpacing pixels of empty space (before the wrench
 //      menu).
 // The reason the container contains the trailing space "s", rather than having
 // it be handled by the parent view, is so that when the chevron is invisible
 // and the user starts dragging an icon around, we have the space to draw the
 // ultimate drop indicator.  (Otherwise, we'd be trying to draw it into the
 // padding beyond our right edge, and it wouldn't appear.)
 //
-// The BrowserActionsContainer follows a few rules, in terms of user experience:
+// The BrowserActionsContainer in 'main' mode follows a few rules, in terms of
+// user experience:
 //
 // 1) The container can never grow beyond the space needed to show all icons
 // (hereby referred to as the max width).
 // 2) The container can never shrink below the space needed to show just the
 // initial padding and the chevron (ignoring the case where there are no icons
 // to show, in which case the container won't be visible anyway).
 // 3) The container snaps into place (to the pixel count that fits the visible
 // icons) to make sure there is no wasted space at the edges of the container.
 // 4) If the user adds or removes icons (read: installs/uninstalls browser
 // actions) we grow and shrink the container as needed - but ONLY if the
 // container was at max width to begin with.
 // 5) If the container is NOT at max width (has an overflow menu), we respect
 // that size when adding and removing icons and DON'T grow/shrink the container.
 // This means that new icons (which always appear at the far right) will show up
-// in the overflow menu. The install bubble for extensions points to the chevron
+// in the overflow. The install bubble for extensions points to the chevron
 // menu in this case.
 //
 // Resizing the BrowserActionsContainer:
 //
 // The ResizeArea view sends OnResize messages to the BrowserActionsContainer
 // class as the user drags it. This modifies the value for |resize_amount_|.
 // That indicates to the container that a resize is in progress and is used to
 // calculate the size in GetPreferredSize(), though that function never exceeds
 // the defined minimum and maximum size of the container.
 //
@@ skipping 23 matching lines @@
     : public views::View,
       public views::MenuButtonListener,
       public views::ResizeAreaDelegate,
       public gfx::AnimationDelegate,
       public extensions::ExtensionToolbarModel::Observer,
       public BrowserActionOverflowMenuController::Observer,
       public views::WidgetObserver,
       public BrowserActionView::Delegate,
       public extensions::ExtensionKeybindingRegistry::Delegate {
  public:
-  BrowserActionsContainer(Browser* browser, views::View* owner_view);
+  // Constructs a BrowserActionContainer for a particular |browser| object, and
+  // specifies which view is the |owner_view|. For documentation of
+  // |main_container_|, see class comments.

[Peter Kasting, 2014/06/30 23:36:34]

Nit: No trailing underscore

[Finnur, 2014/07/02 16:59:57]

Done.

+  BrowserActionsContainer(Browser* browser,
+                          views::View* owner_view,
+                          BrowserActionsContainer* main_container);
   virtual ~BrowserActionsContainer();
 
   void Init();
 
   // Get the number of browser actions being displayed.
   int num_browser_actions() const { return browser_action_views_.size(); }
 
   // Whether we are performing resize animation on the container.
   bool animating() const { return animation_target_size_ > 0; }
 
@@ skipping 79 matching lines @@
       BrowserActionOverflowMenuController* controller) OVERRIDE;
 
   // Overridden from views::WidgetObserver:
   virtual void OnWidgetDestroying(views::Widget* widget) OVERRIDE;
 
   // Overridden from BrowserActionView::Delegate:
   virtual void InspectPopup(ExtensionAction* action) OVERRIDE;
   virtual int GetCurrentTabId() const OVERRIDE;
   virtual void OnBrowserActionExecuted(BrowserActionButton* button) OVERRIDE;
   virtual void OnBrowserActionVisibilityChanged() OVERRIDE;
+  virtual bool ShownInsideMenu() const OVERRIDE;
 
   // Overridden from extension::ExtensionKeybindingRegistry::Delegate:
   virtual extensions::ActiveTabPermissionGranter*
       GetActiveTabPermissionGranter() OVERRIDE;
 
   // Moves a browser action with |id| to |new_index|.
   void MoveBrowserAction(const std::string& extension_id, size_t new_index);
 
   // Shows the popup for |extension| if possible. Returns true if a new popup
   // was shown. Showing the popup will grant tab permissions if
@@ skipping 101 matching lines @@
   bool ShouldDisplayBrowserAction(const extensions::Extension* extension);
 
   // Show a popup. Returns true if a new popup was shown. Showing the popup will
   // grant tab permissions if |grant_tab_permissions| is true. Only pass true
   // for this argument for popups triggered interactively, not popups triggered
   // by an API.
   bool ShowPopup(BrowserActionButton* button,
                  ExtensionPopup::ShowAction show_action,
                  bool grant_tab_permissions);
 
+  // Whether this container is overflow container (as opposed to in 'main'

[Peter Kasting, 2014/06/30 23:36:34]

Nit: overflow container -> in overflow mode (or say "the overflow container" and
change "in 'main' mode" to "the main container").

[Peter Kasting, 2014/07/02 20:54:00]

You should still do this.

[Finnur, 2014/07/03 13:59:31]

Indeed.

+  // mode). See class comments for details on the difference.
+  bool handling_overflow() const { return main_container_ != NULL; }

[Peter Kasting, 2014/06/30 23:36:34]

Nit: handling_overflow() is a rather confusing name IMO.  I'd think
is_overflow_container() or in_overflow_mode() (pick what matches the comment)
would be better.

However, given that this is private, I suggest calling code either just check
whether |main_container_| is non-NULL directly, or call ShownInsideMenu().

[Finnur, 2014/07/02 16:59:56]

Changed it to in_overflow_mode. I think that is the most readable of the
alternatives, as it describes the purpose of the container and at the same time
helps explains the logic a bit (why we are dealing with the icons that the main
container isn't showing).

+
   // The vector of browser actions (icons/image buttons for each action). Note
   // that not every BrowserAction in the ToolbarModel will necessarily be in
   // this collection. Some extensions may be disabled in incognito windows.
   BrowserActionViews browser_action_views_;
 
   Profile* profile_;
 
   // The Browser object the container is associated with.
   Browser* browser_;
 
   // The view that owns us.
   views::View* owner_view_;
 
+  // The main container we are serving as overflow for, or NULL if this
+  // class is the the main container. See class comments for details on
+  // the difference between main and overflow.
+  BrowserActionsContainer* main_container_;
+
   // The current popup and the button it came from.  NULL if no popup.
   ExtensionPopup* popup_;
 
   // The button that triggered the current popup (just a reference to a button
   // from browser_action_views_).
   BrowserActionButton* popup_button_;
 
   // The model that tracks the order of the toolbar icons.
   extensions::ExtensionToolbarModel* model_;
 
   // The current width of the container.
   int container_width_;
 
   // The resize area for the container.
   views::ResizeArea* resize_area_;
 
-  // The chevron for accessing the overflow items.
+  // The chevron for accessing the overflow items. Can be NULL if the toolbar
+  // is permanently suppressing the chevron menu.

[Peter Kasting, 2014/06/30 23:36:34]

Nit: "NULL when in overflow mode or when the toolbar..." or similar.

[Finnur, 2014/07/02 16:59:56]

Done.

   views::MenuButton* chevron_;
 
   // The painter used when we are highlighting a subset of extensions.
   scoped_ptr<views::Painter> highlight_painter_;
 
   // The menu to show for the overflow button (chevron). This class manages its
   // own lifetime so that it can stay alive during drag and drop operations.
   BrowserActionOverflowMenuController* overflow_menu_;
 
   // The animation that happens when the container snaps to place.
@@ skipping 21 matching lines @@
 
   // Handles delayed showing of the overflow menu when hovering.
   base::WeakPtrFactory<BrowserActionsContainer> show_menu_task_factory_;
 
   ObserverList<BrowserActionsContainerObserver> observers_;
 
   DISALLOW_COPY_AND_ASSIGN(BrowserActionsContainer);
 };
 
 #endif  // CHROME_BROWSER_UI_VIEWS_TOOLBAR_BROWSER_ACTIONS_CONTAINER_H_