Bug 35811 – [chromium] need DragImage implementation

• Component: Platform
• Status: RESOLVED FIXED

[Evan Stade]

chromium currently has a stubbed DragImage implementation. We should add one based on Skia.

[Evan Stade]

Created attachment 50127 [details]
try1

[WebKit Review Bot]

Attachment 50127 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/chromium/DragImageRef.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Evan Stade]

Created attachment 50129 [details]
style

[WebKit Review Bot]

Attachment 50127 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/335639

[Evan Stade]

Created attachment 50131 [details]
forgot to svn add

[WebKit Review Bot]

Attachment 50131 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/chromium/DragImageRef.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Evan Stade]

Created attachment 50132 [details]
style, 2

[WebKit Review Bot]

Attachment 50132 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/chromium/DragImageRef.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 50129 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/336632

[Evan Stade]

Created attachment 50136 [details]
mac build fix

[Evan Stade]

Created attachment 50241 [details]
review comments

[Darin Fisher (:fishd, Google)]

Comment on attachment 50241 [details]
review comments

> Index: WebCore/platform/chromium/DragImageChromium.cpp
...
> +#if OS(DARWIN)
> +#include "skia/ext/skia_utils_mac.h"
> +#endif

Can you use CG in the Mac port instead?  We should really avoid using
skia/ext in WebCore.  It introduces a circular dependency between the
repositories!


> +DragImageRef createDragImageFromImage(Image* image)
> +{
> +#if OS(DARWIN)
> +    SkBitmap bitmap = gfx::CGImageToSkBitmap(image->getCGImageRef());

please avoid adding gfx:: namespace stuff to WebCore!  that stuff is
chromium only.


> Index: WebKit/chromium/ChangeLog
...
> +        * public/WebDragImageRef.h: Added.
> +        * src/DragClientImpl.cpp:
> +        (WebKit::DragClientImpl::startDrag):
> +        * src/WebViewImpl.cpp:
> +        (WebKit::WebViewImpl::startDragging):
> +        * src/WebViewImpl.h:

is there some other public interface change missing?  I don't see
any public API that uses WebDragImageRef.

also, please see WebImage.h to note how things differ between
WEBKIT_USING_SKIA and WEBKIT_USING_CG.

It seems like WebDragImageRef should be made consistent with
WebImage.

[Evan Stade]

(In reply to comment #12)
> (From update of attachment 50241 [details])
> > Index: WebCore/platform/chromium/DragImageChromium.cpp
> ...
> > +#if OS(DARWIN)
> > +#include "skia/ext/skia_utils_mac.h"
> > +#endif
> 
> Can you use CG in the Mac port instead?  We should really avoid using
> skia/ext in WebCore.  It introduces a circular dependency between the
> repositories!

we can use CG for Mac, but then we have to implement DragImageChromium twice, and fork a lot of code on the chrome side of this patch.

> 
> 
> > +DragImageRef createDragImageFromImage(Image* image)
> > +{
> > +#if OS(DARWIN)
> > +    SkBitmap bitmap = gfx::CGImageToSkBitmap(image->getCGImageRef());
> 
> please avoid adding gfx:: namespace stuff to WebCore!  that stuff is
> chromium only.

After talking to several mac people, this is the only way I could find to convert CG to SkBitmap. Is there a better way? If not I can leave the mac side unimplemented for the time being.

> 
> 
> > Index: WebKit/chromium/ChangeLog
> ...
> > +        * public/WebDragImageRef.h: Added.
> > +        * src/DragClientImpl.cpp:
> > +        (WebKit::DragClientImpl::startDrag):
> > +        * src/WebViewImpl.cpp:
> > +        (WebKit::WebViewImpl::startDragging):
> > +        * src/WebViewImpl.h:
> 
> is there some other public interface change missing?  I don't see
> any public API that uses WebDragImageRef.
> 
> also, please see WebImage.h to note how things differ between
> WEBKIT_USING_SKIA and WEBKIT_USING_CG.
> 
> It seems like WebDragImageRef should be made consistent with
> WebImage.

Yes, the change to WebViewClient::StartDragging is missing (see FIXME in WebViewImpl). That is not part of this patch because I am breaking it up into multiple pieces.

[Evan Stade]

Created attachment 50269 [details]
use WebImage, no mac implementation

[WebKit Review Bot]

Attachment 50269 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WARNING: Could not read file. Skipping: "WebCore/platform/chromium/DragImageChromium.cpp"
WebCore/platform/chromium/DragImageChromiumSkia.cpp:32:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
WebCore/platform/chromium/DragImageChromiumSkia.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Fisher (:fishd, Google)]

Comment on attachment 50269 [details]
use WebImage, no mac implementation

> Index: WebKit/chromium/public/WebViewClient.h
...
>      virtual void startDragging(
> -        const WebPoint& from, const WebDragData&, WebDragOperationsMask) { }
> +        const WebDragData&, WebDragOperationsMask, const WebImage&, const WebPoint&) { }

Can you avoid making this a two-sided patch landing by continuing
to support the old API temporarily?


> Index: WebKit/chromium/src/DragClientImpl.cpp
...
> +#if WEBKIT_USING_SKIA
> +    m_webView->startDragging(
> +        dragData, static_cast<WebDragOperationsMask>(dragOperationMask),
> +        WebImage(*dragImage), offsetPoint);
> +#else
> +    // FIXME

^^^ Can you add a comment about this FIXME?  Something about needing
to pass a non-empty image perhaps?


Are the style bot issues something to worry about?

[Evan Stade]

(In reply to comment #16)
> (From update of attachment 50269 [details])
> > Index: WebKit/chromium/public/WebViewClient.h
> ...
> >      virtual void startDragging(
> > -        const WebPoint& from, const WebDragData&, WebDragOperationsMask) { }
> > +        const WebDragData&, WebDragOperationsMask, const WebImage&, const WebPoint&) { }
> 
> Can you avoid making this a two-sided patch landing by continuing
> to support the old API temporarily?

we can land the chrome side first now without anything breaking. Sorry, I should have mentioned that here. I only mentioned that at http://codereview.chromium.org/668125/show. So perhaps it would be best to review that side of it first?

> 
> 
> > Index: WebKit/chromium/src/DragClientImpl.cpp
> ...
> > +#if WEBKIT_USING_SKIA
> > +    m_webView->startDragging(
> > +        dragData, static_cast<WebDragOperationsMask>(dragOperationMask),
> > +        WebImage(*dragImage), offsetPoint);
> > +#else
> > +    // FIXME
> 
> ^^^ Can you add a comment about this FIXME?  Something about needing
> to pass a non-empty image perhaps?

ok

> 
> 
> Are the style bot issues something to worry about?

yes

[Evan Stade]

Created attachment 50868 [details]
comment + style

the chrome side was landed. I have updated the patch. Possible to take another look? thanks.

[WebKit Review Bot]

Attachment 50868 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/892042

[Evan Stade]

Created attachment 50908 [details]
try to fix mysterious build failure

don't really understand the build failure. The chromium trybots like the patch ok.

[WebKit Review Bot]

Attachment 50908 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WARNING: Could not read file. Skipping: "WebCore/platform/chromium/DragImageChromium.cpp"
WebCore/platform/chromium/DragImageChromiumSkia.cpp:32:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
WebCore/platform/chromium/DragImageChromiumSkia.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Evan Stade]

Created attachment 50910 [details]
fixed

[Evan Stade]

Created attachment 50912 [details]
correct relative paths

[Darin Fisher (:fishd, Google)]

Comment on attachment 50912 [details]
correct relative paths

r=me

[WebKit Commit Bot]

Comment on attachment 50912 [details]
correct relative paths

Rejecting patch 50912 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Darin Fisher', '--force']" exit_code: 1
Last 500 characters of output:
ching file WebKit/chromium/src/DragClientImpl.cpp
patching file WebKit/chromium/src/WebViewImpl.cpp
Hunk #2 succeeded at 1933 (offset 49 lines).
patching file WebKit/chromium/public/WebViewClient.h
Hunk #1 FAILED at 47.
Hunk #2 succeeded at 223 (offset 3 lines).
1 out of 2 hunks FAILED -- saving rejects to file WebKit/chromium/public/WebViewClient.h.rej
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebKit/chromium/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.

Full output: http://webkit-commit-queue.appspot.com/results/1628184

[Evan Stade]

Created attachment 52339 [details]
re-synced

synced to ToT

[WebKit Commit Bot]

Comment on attachment 52339 [details]
re-synced

Rejecting patch 52339 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--non-interactive', '--build-style=both', '--quiet', '52339', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
webkitpy/tool/commands/stepsequence.py", line 60, in _run
    step(tool, options).run(state)
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 59, in run
    if self._has_valid_reviewer(changelog_entry):
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 41, in _has_valid_reviewer
    if changelog_entry.reviewer():
AttributeError: 'NoneType' object has no attribute 'reviewer'

[Eric Seidel (no email)]

That was not a very elegant error on our part.

webkitpy.common.system.executive.ScriptError: Failed to parse ChangeLog: /Projects/WebKit/WebKit/chromium/ChangeLog

Seems to be the problem.

[Eric Seidel (no email)]

Comment on attachment 52339 [details]
re-synced

Looks like your chromium changelog has a leading + sign.  That would throw our ChangeLog parser off. :)

[Evan Stade]

Created attachment 52426 [details]
plus--

[WebKit Commit Bot]

Comment on attachment 52426 [details]
plus--

Rejecting patch 52426 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', '52426', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
webkitpy/tool/commands/stepsequence.py", line 60, in _run
    step(tool, options).run(state)
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 59, in run
    if self._has_valid_reviewer(changelog_entry):
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 41, in _has_valid_reviewer
    if changelog_entry.reviewer():
AttributeError: 'NoneType' object has no attribute 'reviewer'

[Eric Seidel (no email)]

This looks like another ChangeLog parsing problem.  Again, my apologies for our poor error reporting.

You can test this locally by running:
webkit-patch commit-message

[Evan Stade]

Created attachment 52445 [details]
space--

thanks for the tip

Parsing ChangeLog: /usr/local/google/WebKit/WebKit/chromium/ChangeLog
Parsing ChangeLog: /usr/local/google/WebKit/WebCore/ChangeLog
2010-04-01  Evan Stade  <[e-mail]>

        Reviewed by NOBODY (OOPS!).

        [chromium] need DragImage implementation
        https://bugs.webkit.org/show_bug.cgi?id=35811

        Use the DragImageRef that the DragController passes to us.

        * public/WebViewClient.h:
        (WebKit::WebViewClient::startDragging):
        * src/DragClientImpl.cpp:
        (WebKit::DragClientImpl::startDrag):
        * src/WebViewImpl.cpp:
        (WebKit::WebViewImpl::startDragging):
        * src/WebViewImpl.h:
2010-04-01  Evan Stade  <[e-mail]>

        Reviewed by NOBODY (OOPS!).

        [chromium] need DragImage implementation
        https://bugs.webkit.org/show_bug.cgi?id=35811

        Basic implementation using SkBitmap. Transformations are not supported
        yet. No implementation for mac.

        * WebCore.gyp/WebCore.gyp:
        * WebCore.gypi:
        * platform/chromium/DragImageChromium.cpp:
        * platform/chromium/DragImageRef.h:

[Eric Seidel (no email)]

Comment on attachment 52445 [details]
space--

Sorry our tools have been such trouble.  /me crosses fingers.

[WebKit Commit Bot]

Comment on attachment 52445 [details]
space--

Clearing flags on attachment: 52445

Committed r57028: <http://trac.webkit.org/changeset/57028>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/57028 might have broken Chromium Linux Release

[Evan Stade]

Created attachment 52465 [details]
svn add

these two files got lost in the shuffle

[Adam Barth]

Comment on attachment 52465 [details]
svn add

Clearing flags on attachment: 52465

Committed r57032: <http://trac.webkit.org/changeset/57032>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.

[Darin Fisher (:fishd, Google)]

Just about drag-n-drop related layout test is now failing for the chromium build.  I think this needs to be reverted.

[Nico Weber]

Mac side at https://bugs.webkit.org/show_bug.cgi?id=37069 .

[Pavel Feldman]

(In reply to comment #41)
> Just about drag-n-drop related layout test is now failing for the chromium
> build.  I think this needs to be reverted.

I am on duty starting on Monday. Will revert this unless tests get fixed by Monday morning GMT.

[Nico Weber]

I can look after the tests if someone tells me where the list of now failing tests is.

[Dimitri Glazkov (Google)]

http://build.chromium.org/buildbot/waterfall.fyi/waterfall?branch=&builder=Webkit+(webkit.org)&builder=Webkit+Linux+(webkit.org)&builder=Webkit+Mac+(webkit.org)&builder=XP+Perf+(webkit.org)(1)&builder=XP+Perf+(webkit.org)(2)&builder=Linux+Perf+(webkit.org)

[Nico Weber]

http://codereview.chromium.org/1591013 should do the trick.

I'm having troubles running layout tests locally (see my mail to chromium-dev), so I'm not 100% sure that it does, but it seems very likely that that's the cause.

[Pavel Feldman]

Rolled out both r57028 and r57032.

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	R	WebCore/platform/chromium/DragImageChromiumMac.cpp => WebCore/platform/chromium/DragImageChromium.cpp
	D	WebCore/platform/chromium/DragImageChromiumSkia.cpp
	M	WebCore/ChangeLog
	M	WebCore/WebCore.gyp/WebCore.gyp
	M	WebCore/WebCore.gypi
	M	WebCore/platform/chromium/DragImageRef.h
	M	WebKit/chromium/ChangeLog
	M	WebKit/chromium/public/WebViewClient.h
	M	WebKit/chromium/src/DragClientImpl.cpp
	M	WebKit/chromium/src/WebViewImpl.cpp
	M	WebKit/chromium/src/WebViewImpl.h
Committed r57064

[Evan Stade]

Created attachment 52573 [details]
null check

Fix was null check in DragClientImpl::startDragging

[Nico Weber]

As said on the corresponding chrome bug, not implementing scaleDragImage() destroys the IPC connection when dragging big images, which looks like a renderer crash to the browser. If it's just a couple lines, maybe you can just implement it.

[WebKit Review Bot]

Attachment 52573 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
ERROR: File does not exist: WebCore/platform/chromium/DragImageChromium.cpp


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Chris, this is an bug in check-webkit-style, no?

[Nico Weber]

I retract my comment. I didn't realize that Evan does `return 0;` instead of `return image` in scaleDragImage().

[Evan Stade]

Created attachment 52578 [details]
scale image implemented

Well, I couldn't get it to crash even for large images, but here is a scale implementation anyway (which appears to work, as scale is called on small images).

[WebKit Review Bot]

Attachment 52578 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
ERROR: File does not exist: WebCore/platform/chromium/DragImageChromium.cpp


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Jerdonek]

(In reply to comment #51)
> Chris, this is an bug in check-webkit-style, no?

I think so.  I had originally made it just log a message, but Shinichiro requested that I change it to raise an exception:

https://bugs.webkit.org/show_bug.cgi?id=36957#c3

[Chris Jerdonek]

(In reply to comment #55)
> (In reply to comment #51)
> > Chris, this is an bug in check-webkit-style, no?
> 
> I think so.  I had originally made it just log a message, but Shinichiro
> requested that I change it to raise an exception:
> 
> https://bugs.webkit.org/show_bug.cgi?id=36957#c3

Filed bug here:

https://bugs.webkit.org/show_bug.cgi?id=37122

[Dimitri Glazkov (Google)]

Comment on attachment 52578 [details]
scale image implemented

ok.

[WebKit Commit Bot]

Comment on attachment 52578 [details]
scale image implemented

Clearing flags on attachment: 52578

Committed r57162: <http://trac.webkit.org/changeset/57162>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.