Adjust BrowserActionsContainer drag and drop to work for 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 273 matching lines @@
   // Overridden from views::View:
   virtual void ViewHierarchyChanged(
       const ViewHierarchyChangedDetails& details) OVERRIDE;
   virtual void OnPaint(gfx::Canvas* canvas) OVERRIDE;
   virtual void OnThemeChanged() OVERRIDE;
 
  private:
   friend class BrowserActionView;  // So it can access IconWidth().
   friend class ShowFolderMenuTask;
 
+  // A struct representing the position at which an action will be dropped.
+  struct DropPosition;
+
   typedef std::vector<BrowserActionView*> BrowserActionViews;
 
   // Returns the width of an icon, optionally with its padding.
   static int IconWidth(bool include_padding);
 
   // Returns the height of an icon.
   static int IconHeight();
 
   // extensions::ExtensionToolbarModel::Observer implementation.
   virtual void BrowserActionAdded(const extensions::Extension* extension,
@@ skipping 19 matching lines @@
   void StopShowFolderDropMenuTimer();
 
   // Show the drop down folder after a slight delay.
   void StartShowFolderDropMenuTimer();
 
   // Show the overflow menu.
   void ShowDropFolder();
 
   // Sets the drop indicator position (and schedules paint if the position has
   // changed).
-  void SetDropIndicator(int x_pos);
+  void SetDropPosition(size_t row, size_t icon_in_row);
+
+  // Resets the drop position to NULL (removing the indicator).
+  void ResetDropPosition();
 
   // Given a number of |icons| and whether to |display_chevron|, returns the
   // amount of pixels needed to draw the entire container.  For convenience,
   // callers can set |icons| to -1 to mean "all icons".
   int IconCountToWidth(int icons, bool display_chevron) const;
 
   // Given a pixel width, returns the number of icons that fit.  (This
   // automatically determines whether a chevron will be needed and includes it
   // in the calculation.)
   size_t WidthToIconCount(int pixels) const;
@@ skipping 16 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);
 
+  // Return the index of the first visible icon.
+  size_t GetFirstVisibleIconIndex() const;
+
   // Whether this container is in overflow mode (as opposed to in 'main'
   // mode). See class comments for details on the difference.
   bool in_overflow_mode() const { return main_container_ != NULL; }
 
   // 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_;
@@ skipping 44 matching lines @@
 
   // This is used while the user is resizing (and when the animations are in
   // progress) to know how wide the delta is between the current state and what
   // we should draw.
   int resize_amount_;
 
   // Keeps track of the absolute pixel width the container should have when we
   // are done animating.
   int animation_target_size_;
 
-  // The x position for where to draw the drop indicator. -1 if no indicator.
-  int drop_indicator_position_;
+  // The DropPosition for the current drag-and-drop operation, or NULL if there
+  // is none.
+  scoped_ptr<DropPosition> drop_position_;

[Peter Kasting, 2014/07/21 20:24:09]

I wonder if instead of using NULL to mean "none", this should be a
member-by-value and there should be a "no position" drop position value (much
like npos works for std::string).

[Devlin, 2014/07/22 17:30:47]

I personally prefer the scoped ptr approach, because it easily allows us to just
have a forward declaration for the struct.  Plus, it's a bit more code and
somewhat odd to do an npos style thing, because it's not just POD so we'd have
to have a static method, like:
DropPosition {
  static DropPosition invalid() { return DropPosition(-1, -1); }
};

We also then need to have allow things like == comparison and copying (which we
allow for structs, but generally don't encourage unless necessary).

Is there a deeper reason to go for the alternative?  I'm happy to change it if
so, just wanna make sure it's worth it. :)

[Peter Kasting, 2014/07/22 18:21:11]

Let me back up for a second.  You start by saying this isn't a POD struct.  But
as long as you aren't defining a destructor, copy/assignment operators, etc. --
which you aren't now -- I believe this struct _is_ POD.  That means you don't
need any sort of invalid() definition, and you don't need to write operator==()
(the compiler will do it for you).  You can simply do something like this:

  namespace {
  const DropPosition kInvalidPos(static_cast<size_t>(-1),
static_cast<size_t>(-1));
  }
  ...
  if (position != kInvalidPos) ...

(As long as DropPosition is POD, we're allowed to static-initialize it per the
style guide.)

Assuming all that is true, I think this code is actually simpler and clearer
than the current code, and the only downside is that you have to declare the
struct in the .h file (but the style guide explicitly says not to use pointer
members in a class just to avoid having to declare other classes in a header). 
I'm curious whether you agree.

[Devlin, 2014/07/22 20:29:33]

But, if we define a constructor (default or otherwise), the struct is no longer
POD (pre-C++11), and we can't statically initialize it.  We could omit the
constructor, but then we have a somewhat-awkward requirement to initialize the
struct "by hand" everywhere - which is more code, and more likely to break IMHO.

Thoughts?

[Peter Kasting, 2014/07/22 21:10:00]

Oof, I didn't recall that.

It's funny too, we're about a week away from hopefully being able to start using
C++11 language features.
Well, you can use aggregate initialization, right?

  DropPosition drop_position = { x, y };

If so, this still doesn't seem _too_ bad -- but I'll leave it up to you about
what the overall right way is.  Either solution is OK by me.

[Devlin, 2014/07/22 22:13:20]

Aggregate initialization isn't too bad, but we can't do it in the constructor of
BrowserActionsContainer (i.e., BAC::BAC() : drop_position_({x, y}) isn't
allowed), so we'd still be left with having to initialize by hand there. 
Overall... I think this is cleaner.  We'll revisit in a bit once C++11 is more
fully allowed. :)

 
   // The class that registers for keyboard shortcuts for extension commands.
   scoped_ptr<ExtensionKeybindingRegistryViews> extension_keybinding_registry_;
 
   base::WeakPtrFactory<BrowserActionsContainer> task_factory_;
 
   // 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_