Adjust BrowserActionsContainer drag and drop to work for overflow

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

Index: chrome/browser/ui/views/toolbar/browser_actions_container.h
diff --git a/chrome/browser/ui/views/toolbar/browser_actions_container.h b/chrome/browser/ui/views/toolbar/browser_actions_container.h
index 51a03ef087d0004cf9a7da4227eae1574f677fa9..ec08d4895637703d33b799ffd60cfd2d6e3e9fd8 100644
--- a/chrome/browser/ui/views/toolbar/browser_actions_container.h
+++ b/chrome/browser/ui/views/toolbar/browser_actions_container.h
@@ -291,6 +291,9 @@ class BrowserActionsContainer
   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.
@@ -330,7 +333,10 @@ class BrowserActionsContainer
 
   // 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,
@@ -367,6 +373,9 @@ class BrowserActionsContainer
                  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; }
@@ -431,8 +440,9 @@ class BrowserActionsContainer
   // 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_;