GTK: hook up drag and drop of browser actions (for reordering)....

GTK: hook up drag and drop of browser actions (for reordering).

The changes are propagated across open chrome windows, but are not persisted between sessions yet.

BUG=26990
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=34272

[Evan Stade, 2009-12-08 22:41:40]

derat: gtk bits
erikkay: extensions bits

[Evan Stade, 2009-12-08 22:43:52]

http://codereview.chromium.org/463056/diff/1/4
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/463056/diff/1/4#newcode259
chrome/browser/extensions/extensions_service.cc:259: // The extension may have
been disabled already.
I hit this by double-clicking the disable link on chrome://extensions

[Daniel Erat, 2009-12-09 07:01:56]

http://codereview.chromium.org/463056/diff/1/5
File chrome/browser/extensions/extension_toolbar_model.cc (right):

http://codereview.chromium.org/463056/diff/1/5#newcode38
chrome/browser/extensions/extension_toolbar_model.cc:38: ExtensionList::iterator
pos = std::find(begin(), end(), extension);
add a DCHECK() that it was found?

http://codereview.chromium.org/463056/diff/1/3
File chrome/browser/extensions/extension_toolbar_model.h (right):

http://codereview.chromium.org/463056/diff/1/3#newcode32
chrome/browser/extensions/extension_toolbar_model.h:32: // Ther browser action
button for |extension| has been moved to |index|.
s/Ther/The/

http://codereview.chromium.org/463056/diff/1/7
File chrome/browser/gtk/browser_actions_toolbar_gtk.cc (right):

http://codereview.chromium.org/463056/diff/1/7#newcode42
chrome/browser/gtk/browser_actions_toolbar_gtk.cc:42: drag_target.target =
strdup(kDragTarget);
Needing to free this later seems kinda unfortunate.  Could you do something like
creating a static C++ string inside of this method and setting .target to the
output of its data() method?

http://codereview.chromium.org/463056/diff/1/7#newcode329
chrome/browser/gtk/browser_actions_toolbar_gtk.cc:329: gtk_grab_add(widget());
Why do you need the add before the remove?

[Evan Stade, 2009-12-09 19:45:05]

Dan, would you mind reviewing the rest of the CL as well (I see you already at
least glanced over it)? The extensions dudes are rather busy at the moment and
this is fairly generic stuff.

http://codereview.chromium.org/463056/diff/1/5
File chrome/browser/extensions/extension_toolbar_model.cc (right):

http://codereview.chromium.org/463056/diff/1/5#newcode38
chrome/browser/extensions/extension_toolbar_model.cc:38: ExtensionList::iterator
pos = std::find(begin(), end(), extension);
On 2009/12/09 07:01:56, Daniel Erat wrote:
> add a DCHECK() that it was found?

Done.

http://codereview.chromium.org/463056/diff/1/3
File chrome/browser/extensions/extension_toolbar_model.h (right):

http://codereview.chromium.org/463056/diff/1/3#newcode32
chrome/browser/extensions/extension_toolbar_model.h:32: // Ther browser action
button for |extension| has been moved to |index|.
On 2009/12/09 07:01:56, Daniel Erat wrote:
> s/Ther/The/

Done.

http://codereview.chromium.org/463056/diff/1/7
File chrome/browser/gtk/browser_actions_toolbar_gtk.cc (right):

http://codereview.chromium.org/463056/diff/1/7#newcode42
chrome/browser/gtk/browser_actions_toolbar_gtk.cc:42: drag_target.target =
strdup(kDragTarget);
On 2009/12/09 07:01:56, Daniel Erat wrote:
> Needing to free this later seems kinda unfortunate.  Could you do something
like
> creating a static C++ string inside of this method and setting .target to the
> output of its data() method?

ok

http://codereview.chromium.org/463056/diff/1/7#newcode329
chrome/browser/gtk/browser_actions_toolbar_gtk.cc:329: gtk_grab_add(widget());
On 2009/12/09 07:01:56, Daniel Erat wrote:
> Why do you need the add before the remove?

you're right, I probably don't need to.

[tfarina (gmail-do not use), 2009-12-09 20:32:30]

http://codereview.chromium.org/463056/diff/1/5
File chrome/browser/extensions/extension_toolbar_model.cc (right):

http://codereview.chromium.org/463056/diff/1/5#newcode53
chrome/browser/extensions/extension_toolbar_model.cc:53: index =
toolitems_.size();
nit: static_cast<int>

[Daniel Erat, 2009-12-09 23:01:51]

http://codereview.chromium.org/463056/diff/1/3
File chrome/browser/extensions/extension_toolbar_model.h (right):

http://codereview.chromium.org/463056/diff/1/3#newcode39
chrome/browser/extensions/extension_toolbar_model.h:39: void
BrowserActionReordered(Extension* extension, int index);
The naming difference between this and the observer's method seems arbitrary
(BrowserActionMoved vs. BrowserActionReordered).  Can you make them the same, or
maybe use something like "NotifyBrowserActionMoved()" here to make them
distinct?

http://codereview.chromium.org/463056/diff/1/7
File chrome/browser/gtk/browser_actions_toolbar_gtk.cc (right):

http://codereview.chromium.org/463056/diff/1/7#newcode272
chrome/browser/gtk/browser_actions_toolbar_gtk.cc:272: free(drag_target.target);
Assuming that you removed lines like these in response to previous feedback
(reupload to review site if you want me to take another look).

http://codereview.chromium.org/463056/diff/1/7#newcode332
chrome/browser/gtk/browser_actions_toolbar_gtk.cc:332: // Re-generate the
toolbar to clean up unfinished drag business.
Can you elaborate on what the "unfinished drag business" is?

http://codereview.chromium.org/463056/diff/1/7#newcode358
chrome/browser/gtk/browser_actions_toolbar_gtk.cc:358:
gtk_drag_set_icon_pixbuf(drag_context, pixbuf, 0, 0);
Is this really the only way to retain a normal cursor?

http://codereview.chromium.org/463056/diff/1/7#newcode361
chrome/browser/gtk/browser_actions_toolbar_gtk.cc:361: drag_button_ = button;
DCHECK() that drag_button_ isn't already set?

[Evan Stade, 2009-12-09 23:11:51]

actually re-upped this time

http://codereview.chromium.org/463056/diff/1/3
File chrome/browser/extensions/extension_toolbar_model.h (right):

http://codereview.chromium.org/463056/diff/1/3#newcode39
chrome/browser/extensions/extension_toolbar_model.h:39: void
BrowserActionReordered(Extension* extension, int index);
On 2009/12/09 23:01:52, Daniel Erat wrote:
> The naming difference between this and the observer's method seems arbitrary
> (BrowserActionMoved vs. BrowserActionReordered).  Can you make them the same,
or
> maybe use something like "NotifyBrowserActionMoved()" here to make them
> distinct?

Done.

http://codereview.chromium.org/463056/diff/1/7
File chrome/browser/gtk/browser_actions_toolbar_gtk.cc (right):

http://codereview.chromium.org/463056/diff/1/7#newcode332
chrome/browser/gtk/browser_actions_toolbar_gtk.cc:332: // Re-generate the
toolbar to clean up unfinished drag business.
On 2009/12/09 23:01:52, Daniel Erat wrote:
> Can you elaborate on what the "unfinished drag business" is?

Done.

http://codereview.chromium.org/463056/diff/1/7#newcode358
chrome/browser/gtk/browser_actions_toolbar_gtk.cc:358:
gtk_drag_set_icon_pixbuf(drag_context, pixbuf, 0, 0);
On 2009/12/09 23:01:52, Daniel Erat wrote:
> Is this really the only way to retain a normal cursor?

it's the only way we've found (this code is used in tab dragging as well)

http://codereview.chromium.org/463056/diff/1/7#newcode361
chrome/browser/gtk/browser_actions_toolbar_gtk.cc:361: drag_button_ = button;
On 2009/12/09 23:01:52, Daniel Erat wrote:
> DCHECK() that drag_button_ isn't already set?

ok (although it's guaranteed that drag-begin signals will be paired to drag-end
signals)

[Daniel Erat, 2009-12-10 02:42:50]

LGTM

Thanks!