GTK: implement extension bookmark manager drag api....

GTK: implement extension bookmark manager drag api.

Also, cross-platform: eschew RenderViewHostDelegate:BookmarkDrag::DragData type in favor of BookmarkDragData.

BUG=37702
TEST=manual


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

[Evan Stade, 2010-03-17 23:32:26]

Elliot: gtk
Erik: ~gtk

[arv (Not doing code reviews), 2010-03-17 23:37:27]

Adding Mike who did the Mac implementation
Adding Scott who did the initial code review

[Elliot Glaysher, 2010-03-17 23:39:41]

GTK stuff LGTM.

http://codereview.chromium.org/1029004/diff/19001/20011
File chrome/browser/gtk/custom_drag.h (right):

http://codereview.chromium.org/1029004/diff/19001/20011#newcode33
chrome/browser/gtk/custom_drag.h:33: // Since this uses a virtual function, we
can't use a macro.
Too bad.

http://codereview.chromium.org/1029004/diff/19001/20004
File chrome/browser/gtk/menu_gtk.cc (right):

http://codereview.chromium.org/1029004/diff/19001/20004#newcode472
chrome/browser/gtk/menu_gtk.cc:472: gint monitor =
gdk_screen_get_monitor_at_point(screen, *x, *y);
Is this left over from that other patch you were doing?

[Evan Stade, 2010-03-17 23:50:41]

http://codereview.chromium.org/1029004/diff/19001/20004
File chrome/browser/gtk/menu_gtk.cc (right):

http://codereview.chromium.org/1029004/diff/19001/20004#newcode472
chrome/browser/gtk/menu_gtk.cc:472: gint monitor =
gdk_screen_get_monitor_at_point(screen, *x, *y);
On 2010/03/17 23:39:41, Elliot Glaysher wrote:
> Is this left over from that other patch you were doing?

yes. thanks for catching that.

[arv (Not doing code reviews), 2010-03-18 00:00:10]

http://codereview.chromium.org/1029004/diff/19001/20016
File chrome/browser/bookmarks/bookmark_drag_data.cc (right):

http://codereview.chromium.org/1029004/diff/19001/20016#newcode102
chrome/browser/bookmarks/bookmark_drag_data.cc:102: return true;
Does anyone care about the return value?

http://codereview.chromium.org/1029004/diff/19001/20002
File chrome/browser/tab_contents/web_drag_dest_gtk.cc (right):

http://codereview.chromium.org/1029004/diff/19001/20002#newcode179
chrome/browser/tab_contents/web_drag_dest_gtk.cc:179:
bookmark_drag_data_.SetOriginatingProfile(tab_contents_->profile());
When is WebDragDestGtk::OnDragDataReceived called? My concern here is that we
are setting the profile to the browser we are currently dragging over instead of
the original browser.

[sky, 2010-03-18 00:03:38]

LGTM

http://codereview.chromium.org/1029004/diff/19001/20016
File chrome/browser/bookmarks/bookmark_drag_data.cc (right):

http://codereview.chromium.org/1029004/diff/19001/20016#newcode287
chrome/browser/bookmarks/bookmark_drag_data.cc:287: if (profile)
This should DCHECK on profile, or reset profile_path_ if profile is null.

http://codereview.chromium.org/1029004/diff/19001/20017
File chrome/browser/bookmarks/bookmark_drag_data.h (right):

http://codereview.chromium.org/1029004/diff/19001/20017#newcode90
chrome/browser/bookmarks/bookmark_drag_data.h:90: bool ReadFromVector(const
std::vector<const BookmarkNode*>& nodes);
Why does this have a return value?

http://codereview.chromium.org/1029004/diff/33001/17011
File chrome/browser/gtk/custom_drag.h (right):

http://codereview.chromium.org/1029004/diff/33001/17011#newcode12
chrome/browser/gtk/custom_drag.h:12: #include "app/gtk_signal.h"
sort includes.

[Evan Stade, 2010-03-18 00:26:05]

http://codereview.chromium.org/1029004/diff/19001/20016
File chrome/browser/bookmarks/bookmark_drag_data.cc (right):

http://codereview.chromium.org/1029004/diff/19001/20016#newcode102
chrome/browser/bookmarks/bookmark_drag_data.cc:102: return true;
On 2010/03/18 00:00:10, arv wrote:
> Does anyone care about the return value?

I did this to make it analogous to the other Read* functions, but I guess it
should check for nodes.empty() and return false in that case. But no, no one
cares about the return value at this point.

http://codereview.chromium.org/1029004/diff/19001/20016#newcode287
chrome/browser/bookmarks/bookmark_drag_data.cc:287: if (profile)
On 2010/03/18 00:03:38, sky wrote:
> This should DCHECK on profile, or reset profile_path_ if profile is null.

if profile is NULL then profile_path_ will remain empty (same as clearing it).

http://codereview.chromium.org/1029004/diff/19001/20017
File chrome/browser/bookmarks/bookmark_drag_data.h (right):

http://codereview.chromium.org/1029004/diff/19001/20017#newcode90
chrome/browser/bookmarks/bookmark_drag_data.h:90: bool ReadFromVector(const
std::vector<const BookmarkNode*>& nodes);
On 2010/03/18 00:03:38, sky wrote:
> Why does this have a return value?

consistency with other Read* methods. (See my comment in response to arv)

http://codereview.chromium.org/1029004/diff/19001/20002
File chrome/browser/tab_contents/web_drag_dest_gtk.cc (right):

http://codereview.chromium.org/1029004/diff/19001/20002#newcode179
chrome/browser/tab_contents/web_drag_dest_gtk.cc:179:
bookmark_drag_data_.SetOriginatingProfile(tab_contents_->profile());
On 2010/03/18 00:00:10, arv wrote:
> When is WebDragDestGtk::OnDragDataReceived called? My concern here is that we
> are setting the profile to the browser we are currently dragging over instead
of
> the original browser.

oh, hmm. Yes we are setting it to the profile of the current browser. But this
path will only be hit for drags within the current browser anyway. If you tried
to drag from a browser running a different profile it just wouldn't work. I
guess fixing that should be a TODO. How often do you expect that to happen
though?

http://codereview.chromium.org/1029004/diff/33001/17011
File chrome/browser/gtk/custom_drag.h (right):

http://codereview.chromium.org/1029004/diff/33001/17011#newcode12
chrome/browser/gtk/custom_drag.h:12: #include "app/gtk_signal.h"
On 2010/03/18 00:03:38, sky wrote:
> sort includes.

Done.

[arv (Not doing code reviews), 2010-03-18 00:31:54]

http://codereview.chromium.org/1029004/diff/19001/20002
File chrome/browser/tab_contents/web_drag_dest_gtk.cc (right):

http://codereview.chromium.org/1029004/diff/19001/20002#newcode179
chrome/browser/tab_contents/web_drag_dest_gtk.cc:179:
bookmark_drag_data_.SetOriginatingProfile(tab_contents_->profile());
On 2010/03/18 00:26:06, Evan Stade wrote:
> On 2010/03/18 00:00:10, arv wrote:
> > When is WebDragDestGtk::OnDragDataReceived called? My concern here is that
we
> > are setting the profile to the browser we are currently dragging over
instead
> of
> > the original browser.
> 
> oh, hmm. Yes we are setting it to the profile of the current browser. But this
> path will only be hit for drags within the current browser anyway. If you
tried
> to drag from a browser running a different profile it just wouldn't work. 

Why not?

> I
> guess fixing that should be a TODO. How often do you expect that to happen
> though?

I used that on Windows when testing this code path. The old bookmark manager
supported this. I'm fine with making this a TODO but I would like to understand
why this would not work.

[Evan Stade, 2010-03-18 00:40:41]

http://codereview.chromium.org/1029004/diff/19001/20002
File chrome/browser/tab_contents/web_drag_dest_gtk.cc (right):

http://codereview.chromium.org/1029004/diff/19001/20002#newcode179
chrome/browser/tab_contents/web_drag_dest_gtk.cc:179:
bookmark_drag_data_.SetOriginatingProfile(tab_contents_->profile());
On 2010/03/18 00:31:54, arv wrote:
> On 2010/03/18 00:26:06, Evan Stade wrote:
> > On 2010/03/18 00:00:10, arv wrote:
> > > When is WebDragDestGtk::OnDragDataReceived called? My concern here is that
> we
> > > are setting the profile to the browser we are currently dragging over
> instead
> > of
> > > the original browser.
> > 
> > oh, hmm. Yes we are setting it to the profile of the current browser. But
this
> > path will only be hit for drags within the current browser anyway. If you
> tried
> > to drag from a browser running a different profile it just wouldn't work. 
> 
> Why not?
> 
> > I
> > guess fixing that should be a TODO. How often do you expect that to happen
> > though?
> 
> I used that on Windows when testing this code path. The old bookmark manager
> supported this. I'm fine with making this a TODO but I would like to
understand
> why this would not work.
> 
> 

targets that start with CHROME_ are added with GTK_TARGET_SAME_APP meaning they
are only exposed within the app. I'm not sure how much work it would be to
support this.

On Linux, dragging between (old) bookmark managers from different profiles
doesn't currently work, and AFAIK no one has ever complained about it (on the
bug tracker anyway).

[arv (Not doing code reviews), 2010-03-18 00:43:00]

LGTM

I agree that dragging between two profiles is of low priority.