Basic DragImage implementation....

Basic DragImage implementation.

Only the chromium part is for review. The webkit part shows how that will look when I create the patch for webkit later. This can be landed without the change to webkit.

TODO later:
- use the image on windows, mac
- implement the other DragImageRef functions

TEST=drag an image from the render view in GTK
BUG=11457
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=41799

[Evan Stade, 2010-03-05 00:58:17]

I don't think I can break this up into two half patches that don't depend on
each other, so when landing the webkit side, I will have to inform the webkit
roller guy of the chrome side.

[Evan Stade, 2010-03-05 01:33:00]

> I don't think I can break this up into two half patches that don't depend on
> each other

ignore this comment^

[tony, 2010-03-05 03:13:08]

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

http://codereview.chromium.org/668125/diff/1/7#newcode312
chrome/browser/gtk/tab_contents_drag_source.cc:312:
gtk_drag_set_icon_pixbuf(drag_context, pixbuf,
Does gtk_drag_set_icon_pixbuf take ownership of |pixbuf|?

http://codereview.chromium.org/668125/diff/1/8
File chrome/browser/gtk/tab_contents_drag_source.h (right):

http://codereview.chromium.org/668125/diff/1/8#newcode20
chrome/browser/gtk/tab_contents_drag_source.h:20: class SkBitmap;
Nit: You don't need this because of the include above.

http://codereview.chromium.org/668125/diff/1/8#newcode39
chrome/browser/gtk/tab_contents_drag_source.h:39: const gfx::Point
image_offset);
Nit: Do you mean to pass this by reference or is the const unnecessary?

http://codereview.chromium.org/668125/diff/1/4
File chrome/browser/tab_contents/interstitial_page.cc (right):

http://codereview.chromium.org/668125/diff/1/4#newcode271
chrome/browser/tab_contents/interstitial_page.cc:271: //     
DCHECK(!resource_dispatcher_host_notified_);
Do you mean to comment this out?

http://codereview.chromium.org/668125/diff/1/15
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/668125/diff/1/15#newcode1921
chrome/renderer/render_view.cc:1921: image ? *image : SkBitmap(),
Do we need to delete image here?

http://codereview.chromium.org/668125/diff/1/21
File third_party/WebKit/WebCore/platform/chromium/DragImageChromium.cpp (right):

http://codereview.chromium.org/668125/diff/1/21#newcode60
third_party/WebKit/WebCore/platform/chromium/DragImageChromium.cpp:60:
DragImageRef dissolveDragImageToFraction(DragImageRef image, float alpha)
Nit: I think it is webkit style to not name unused params.

http://codereview.chromium.org/668125/diff/1/21#newcode71
third_party/WebKit/WebCore/platform/chromium/DragImageChromium.cpp:71: return
dragImage;
Who owns and deletes this?

http://codereview.chromium.org/668125/diff/1/22
File third_party/WebKit/WebCore/platform/chromium/DragImageRef.h (right):

http://codereview.chromium.org/668125/diff/1/22#newcode36
third_party/WebKit/WebCore/platform/chromium/DragImageRef.h:36: typedef
NativeImageSkia* DragImageRef;
I bet this will be CG for mac.

[darin (slow to review), 2010-03-05 08:09:41]

can you do something clever like what jam did here:
http://codereview.chromium.org/669145/diff/1002/1003

the macro is only defined once the webkit roll happens.  then he can go back
later and clean out the macro.

-darin



On Thu, Mar 4, 2010 at 4:58 PM, <estade@chromium.org> wrote:

> Reviewers: tony, darin,
>
> Message:
> I don't think I can break this up into two half patches that don't depend
> on
> each other, so when landing the webkit side, I will have to inform the
> webkit
> roller guy of the chrome side.
>
> Description:
> Basic DragImage implementation.
>
> TODO before landing:
> - break this up into a two-sided patch
> - fix build on windows, mac, linux views
>
> TODO later:
> - use the image on windows, mac
> - implement the other DragImageRef function
>
> TEST=drag an image from the render view in GTK
> BUG=11457
>
>
> Please review this at http://codereview.chromium.org/668125
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
>  M     chrome/browser/extensions/extension_host.h
>
>  M     chrome/browser/extensions/extension_host.cc
>  M     chrome/browser/gtk/notifications/balloon_view_host_gtk.h
>  M     chrome/browser/gtk/tab_contents_drag_source.h
>  M     chrome/browser/gtk/tab_contents_drag_source.cc
>  M     chrome/browser/renderer_host/render_view_host.h
>
>  M     chrome/browser/renderer_host/render_view_host.cc
>  M     chrome/browser/renderer_host/render_view_host_delegate.h
>  M     chrome/browser/tab_contents/interstitial_page.cc
>  M     chrome/browser/tab_contents/tab_contents_view_gtk.h
>  M     chrome/browser/tab_contents/tab_contents_view_gtk.cc
>  chrome/common/render_messages_internal.h
>  M     chrome/renderer/render_view.h
>  M     chrome/renderer/render_view.cc
>  M     third_party/WebKit/WebCore/platform/chromium/DragImageChromium.cpp
>  M     third_party/WebKit/WebCore/platform/chromium/DragImageRef.h
>  A     third_party/WebKit/WebKit/chromium/public/WebDragImageRef.h
>  M     third_party/WebKit/WebKit/chromium/public/WebViewClient.h
>  M     third_party/WebKit/WebKit/chromium/src/DragClientImpl.cpp
>  third_party/WebKit/WebKit/chromium/src/WebViewImpl.h
>  M     third_party/WebKit/WebKit/chromium/src/WebViewImpl.cpp
>
>
>

[Avi (use Gerrit), 2010-03-05 14:51:08]

http://codereview.chromium.org/668125/diff/1/22
File third_party/WebKit/WebCore/platform/chromium/DragImageRef.h (right):

http://codereview.chromium.org/668125/diff/1/22#newcode36
third_party/WebKit/WebCore/platform/chromium/DragImageRef.h:36: typedef
NativeImageSkia* DragImageRef;
On 2010/03/05 03:13:08, tony wrote:
> I bet this will be CG for mac.

Can we use a typedef like NativeImagePtr from platform/graphics/ImageSource.h ?

[Evan Stade, 2010-03-05 19:53:20]

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

http://codereview.chromium.org/668125/diff/1/7#newcode312
chrome/browser/gtk/tab_contents_drag_source.cc:312:
gtk_drag_set_icon_pixbuf(drag_context, pixbuf,
On 2010/03/05 03:13:08, tony wrote:
> Does gtk_drag_set_icon_pixbuf take ownership of |pixbuf|?

I thought I read that it assumed a reference. But now that I am reading the
code, it looks like it adds its own ref. So we do need to unref.

http://codereview.chromium.org/668125/diff/1/8
File chrome/browser/gtk/tab_contents_drag_source.h (right):

http://codereview.chromium.org/668125/diff/1/8#newcode20
chrome/browser/gtk/tab_contents_drag_source.h:20: class SkBitmap;
On 2010/03/05 03:13:08, tony wrote:
> Nit: You don't need this because of the include above.

Done.

http://codereview.chromium.org/668125/diff/1/8#newcode39
chrome/browser/gtk/tab_contents_drag_source.h:39: const gfx::Point
image_offset);
On 2010/03/05 03:13:08, tony wrote:
> Nit: Do you mean to pass this by reference or is the const unnecessary?

former

http://codereview.chromium.org/668125/diff/1/4
File chrome/browser/tab_contents/interstitial_page.cc (right):

http://codereview.chromium.org/668125/diff/1/4#newcode35
chrome/browser/tab_contents/interstitial_page.cc:35: 
this is also unintentional

http://codereview.chromium.org/668125/diff/1/4#newcode271
chrome/browser/tab_contents/interstitial_page.cc:271: //     
DCHECK(!resource_dispatcher_host_notified_);
On 2010/03/05 03:13:08, tony wrote:
> Do you mean to comment this out?

whoops no. Thanks for catch.

http://codereview.chromium.org/668125/diff/1/15
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/668125/diff/1/15#newcode1921
chrome/renderer/render_view.cc:1921: image ? *image : SkBitmap(),
On 2010/03/05 03:13:08, tony wrote:
> Do we need to delete image here?

no, DragController owns it

http://codereview.chromium.org/668125/diff/1/21
File third_party/WebKit/WebCore/platform/chromium/DragImageChromium.cpp (right):

http://codereview.chromium.org/668125/diff/1/21#newcode60
third_party/WebKit/WebCore/platform/chromium/DragImageChromium.cpp:60:
DragImageRef dissolveDragImageToFraction(DragImageRef image, float alpha)
On 2010/03/05 03:13:08, tony wrote:
> Nit: I think it is webkit style to not name unused params.

ok

http://codereview.chromium.org/668125/diff/1/21#newcode71
third_party/WebKit/WebCore/platform/chromium/DragImageChromium.cpp:71: return
dragImage;
On 2010/03/05 03:13:08, tony wrote:
> Who owns and deletes this?

the caller owns it. They delete it with deleteDragImage. When scaleDragImage or
dissolveDragImage are called, ownership is transferred to that function, and
ownership of the returned pointer (which may or may not be different) is given
to the caller.

http://codereview.chromium.org/668125/diff/1/22
File third_party/WebKit/WebCore/platform/chromium/DragImageRef.h (right):

http://codereview.chromium.org/668125/diff/1/22#newcode36
third_party/WebKit/WebCore/platform/chromium/DragImageRef.h:36: typedef
NativeImageSkia* DragImageRef;
On 2010/03/05 14:51:08, Avi wrote:
> On 2010/03/05 03:13:08, tony wrote:
> > I bet this will be CG for mac.
> 
> Can we use a typedef like NativeImagePtr from platform/graphics/ImageSource.h
?

Talked to Avi, agreed on using skia for mac.

For reference, the reasoning is:
all it is used for is to apply some transformations and then send the bits via
ipc to the browser. At that point mac can convert from skbitmap to CGWhatever.
Mac already uses skia in some places for similar reasons.

I think the actual typedef might have to be SkBitmap though, not
NativeImageSkia:

.../third_party/WebKit/WebCore/WebCore.gyp/../platform/chromium/DragImageChromium.cpp:
In function 'NativeImageSkia*
WebCore::createDragImageFromImage(WebCore::Image*)':
.../third_party/WebKit/WebCore/WebCore.gyp/../platform/chromium/DragImageChromium.cpp:68:error:
cannot convert 'CGImage*' to 'SkBitmap*' in initialization

[tony, 2010-03-08 01:15:40]

Chrome side code LGTM

http://codereview.chromium.org/668125/diff/1/21
File third_party/WebKit/WebCore/platform/chromium/DragImageChromium.cpp (right):

http://codereview.chromium.org/668125/diff/1/21#newcode71
third_party/WebKit/WebCore/platform/chromium/DragImageChromium.cpp:71: return
dragImage;
On 2010/03/05 19:53:20, Evan Stade wrote:
> On 2010/03/05 03:13:08, tony wrote:
> > Who owns and deletes this?
> 
> the caller owns it. They delete it with deleteDragImage. When scaleDragImage
or
> dissolveDragImage are called, ownership is transferred to that function, and
> ownership of the returned pointer (which may or may not be different) is given
> to the caller.

Nit: Can you add a comment saying that deleteDragImage should delete the image?

http://codereview.chromium.org/668125/diff/109/1139#newcode79
third_party/WebKit/WebCore/platform/chromium/DragImageChromium.cpp:79: 
Nit: I think you can just pass *bitmapRef into SkBitmap().

[Evan Stade, 2010-03-08 19:40:09]

webkit side is https://bugs.webkit.org/show_bug.cgi?id=35811

[Evan Stade, 2010-03-09 03:14:12]

updated to use WebImage instead of WebDragImageRef. It should be possible to
land the chrome side of this without the webkit side.

[Evan Stade, 2010-03-11 17:01:29]

ping

[tony, 2010-03-12 00:39:12]

Sorry for missing this.  LGTM!

[darin (slow to review), 2010-03-12 00:51:13]

http://codereview.chromium.org/668125/diff/22002/6015
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/668125/diff/22002/6015#newcode1903
chrome/renderer/render_view.cc:1903: Send(new
ViewHostMsg_StartDragging(routing_id_,
instead of replicating the Send and the message creation, how about just
initializing an SkBitmap local variable only in the WEBKIT_USING_SKIA case?

[Evan Stade, 2010-03-16 00:04:52]

On 2010/03/12 00:51:13, darin wrote:
> http://codereview.chromium.org/668125/diff/22002/6015
> File chrome/renderer/render_view.cc (right):
> 
> http://codereview.chromium.org/668125/diff/22002/6015#newcode1903
> chrome/renderer/render_view.cc:1903: Send(new
> ViewHostMsg_StartDragging(routing_id_,
> instead of replicating the Send and the message creation, how about just
> initializing an SkBitmap local variable only in the WEBKIT_USING_SKIA case?

done.

[darin (slow to review), 2010-03-16 03:55:39]

Thanks!  LGTM

On Mon, Mar 15, 2010 at 5:04 PM, <estade@chromium.org> wrote:

> On 2010/03/12 00:51:13, darin wrote:
>
>> http://codereview.chromium.org/668125/diff/22002/6015
>> File chrome/renderer/render_view.cc (right):
>>
>
>  http://codereview.chromium.org/668125/diff/22002/6015#newcode1903
>> chrome/renderer/render_view.cc:1903: Send(new
>> ViewHostMsg_StartDragging(routing_id_,
>> instead of replicating the Send and the message creation, how about just
>> initializing an SkBitmap local variable only in the WEBKIT_USING_SKIA
>> case?
>>
>
> done.
>
>
>
> http://codereview.chromium.org/668125
>