Implement dragging icon for dragging bookmarks in views/gtk.

Implement dragging icon for dragging bookmarks in views/gtk.

[sky, 2009-10-12 18:10:36]

http://codereview.chromium.org/270068/diff/1008/1009
File app/os_exchange_data_provider_gtk.cc (right):

http://codereview.chromium.org/270068/diff/1008/1009#newcode34
Line 34: if (drag_image_) {
nit: nuke {}

http://codereview.chromium.org/270068/diff/1008/1009#newcode233
Line 233: if (drag_image_) {
nit: no {}

http://codereview.chromium.org/270068/diff/1008/1009#newcode243
Line 243: return drag_image_;
As this just returns the value, you should inline it and go with
unix_hacker_case.

http://codereview.chromium.org/270068/diff/1008/1010
File app/os_exchange_data_provider_gtk.h (right):

http://codereview.chromium.org/270068/diff/1008/1010#newcode75
Line 75: GdkPixbuf* GetDragImage() const;
Can you document ownership here.

http://codereview.chromium.org/270068/diff/1008/1010#newcode76
Line 76: int cursor_offset_x() const {
nit: could you fit this on one line. Same thing with cursor_offset_y.

http://codereview.chromium.org/270068/diff/1008/1013
File views/widget/widget_gtk.cc (right):

http://codereview.chromium.org/270068/diff/1008/1013#newcode197
Line 197: gtk_drag_set_icon_pixbuf(context,
Shouldn't you make sure the drag image is non-null before doing this?

[skrul, 2009-10-12 19:39:03]

http://codereview.chromium.org/270068/diff/1008/1009
File app/os_exchange_data_provider_gtk.cc (right):

http://codereview.chromium.org/270068/diff/1008/1009#newcode34
Line 34: if (drag_image_) {
On 2009/10/12 18:10:37, sky wrote:
> nit: nuke {}

Done.

http://codereview.chromium.org/270068/diff/1008/1009#newcode233
Line 233: if (drag_image_) {
On 2009/10/12 18:10:37, sky wrote:
> nit: no {}

Done.

http://codereview.chromium.org/270068/diff/1008/1009#newcode243
Line 243: return drag_image_;
On 2009/10/12 18:10:37, sky wrote:
> As this just returns the value, you should inline it and go with
> unix_hacker_case.

Done.

http://codereview.chromium.org/270068/diff/1008/1010
File app/os_exchange_data_provider_gtk.h (right):

http://codereview.chromium.org/270068/diff/1008/1010#newcode75
Line 75: GdkPixbuf* GetDragImage() const;
On 2009/10/12 18:10:37, sky wrote:
> Can you document ownership here.

Done.

http://codereview.chromium.org/270068/diff/1008/1010#newcode76
Line 76: int cursor_offset_x() const {
On 2009/10/12 18:10:37, sky wrote:
> nit: could you fit this on one line. Same thing with cursor_offset_y.

Done.

http://codereview.chromium.org/270068/diff/1008/1013
File views/widget/widget_gtk.cc (right):

http://codereview.chromium.org/270068/diff/1008/1013#newcode197
Line 197: gtk_drag_set_icon_pixbuf(context,
On 2009/10/12 18:10:37, sky wrote:
> Shouldn't you make sure the drag image is non-null before doing this?

Done.

[sky, 2009-10-12 19:41:20]

LGTM with the following change.

http://codereview.chromium.org/270068/diff/10/12
File app/os_exchange_data_provider_gtk.h (right):

http://codereview.chromium.org/270068/diff/10/12#newcode77
Line 77: GdkPixbuf* get_drag_image() const { return drag_image_; }
nit: no get, just drag_image, like the other methods you're adding.

[skrul, 2009-10-12 19:44:38]

http://codereview.chromium.org/270068/diff/10/12
File app/os_exchange_data_provider_gtk.h (right):

http://codereview.chromium.org/270068/diff/10/12#newcode77
Line 77: GdkPixbuf* get_drag_image() const { return drag_image_; }
On 2009/10/12 19:41:20, sky wrote:
> nit: no get, just drag_image, like the other methods you're adding.

Gah, sorry, thanks for paying more attention than I am :)