Bug 89688 – Drag image for image elements should be scaled with device scale factor.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Varun Jain]

[Chromium] Drag image for image elements should be scaled with device scale factor.

[Varun Jain]

Created attachment 148877 [details]
Patch

[Adam Barth]

Comment on attachment 148877 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148877&action=review

> Source/WebCore/page/DragController.cpp:855
> +#if PLATFORM(CHROMIUM)

This file shouldn't have any PLATFORM(CHROMIUM) ifdefs.

[Adam Barth]

Please don't use the [Chromium] tag for patches that touch cross-platform code.

[Varun Jain]

(In reply to comment #2)
> (From update of attachment 148877 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148877&action=review
> 
> > Source/WebCore/page/DragController.cpp:855
> > +#if PLATFORM(CHROMIUM)
> 
> This file shouldn't have any PLATFORM(CHROMIUM) ifdefs.

Hi Adam, is your objection to simply the PLATFORM(CHROMIUM) (in which case, I can change it to #if !PLATFORM(MAC) && !PLATFORM(WIN) as in Frame.cpp from my previous CL)... or you are suggesting that DragController is the wrong place to do this?

[Adam Barth]

I haven't looked at the patch in detail.  I can just tell you that it's not correct to have ifdef PLATFORM(CHROMIUM) in this file.

As for using "#if !PLATFORM(MAC) && !PLATFORM(WIN)", that's likely wrong too.  What is it about these two platforms that cause us to need to compile different code?  It's more likely we should create a USE macro that explains the difference.

[Varun Jain]

Created attachment 149080 [details]
Patch

[Varun Jain]

Created attachment 149088 [details]
Patch

[Varun Jain]

(In reply to comment #5)
> I haven't looked at the patch in detail.  I can just tell you that it's not correct to have ifdef PLATFORM(CHROMIUM) in this file.
> 
> As for using "#if !PLATFORM(MAC) && !PLATFORM(WIN)", that's likely wrong too.  What is it about these two platforms that cause us to need to compile different code?  It's more likely we should create a USE macro that explains the difference.

Hi Adam.. I have added a new USE flag as you suggested. PTAL

[Adam Barth]

Thanks.  I guess I don't really understand why Chromium needs to perform this transformation but other ports don't.  What's different about the Chromium port that causes Chromium (and no other ports) to need to do this?

[Dana Jansens]

Comment on attachment 149088 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149088&action=review

> Source/WTF/wtf/Platform.h:1143
> +/* On Chromium, we apply device scale to all drag images */

who is "we" in this sentence, maybe it can be more verbose?

> Source/WebCore/page/DragController.cpp:909
> +    imgRect.setHeight(imgRect.height() * deviceScaleFactor);
> +    imgRect.setWidth(imgRect.width() * deviceScaleFactor);

For non-integer scale factors, is this supposed to truncate? round? expand out? (probably the latter is correct?)

Maybe you can construct a FloatRect and then use its enclosingIntRect?

[WebKit Review Bot]

Comment on attachment 149088 [details]
Patch

Attachment 149088 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13084123

New failing tests:
platform/chromium/compositing/layout-width-change.html
platform/chromium/compositing/render-surface-alpha-blending.html
platform/chromium/compositing/tiny-layer-rotated.html
platform/chromium/compositing/huge-layer-rotated.html
fast/loader/loadInProgress.html
platform/chromium/compositing/3d-corners.html
platform/chromium/compositing/video-frame-size-change.html
platform/chromium/compositing/perpendicular-layer-sorting.html
fast/loader/unload-form-post-about-blank.html
platform/chromium/compositing/child-layer-3d-sorting.html
platform/chromium/compositing/accelerated-drawing/svg-filters.html
platform/chromium/compositing/lost-compositor-context-permanently.html
platform/chromium/compositing/filters/background-filter-blur-outsets.html
platform/chromium/compositing/lost-compositor-context-with-video.html
platform/chromium/compositing/accelerated-drawing/alpha.html
http/tests/xmlhttprequest/zero-length-response.html
http/tests/security/script-crossorigin-loads-correctly.html
platform/chromium/compositing/plugins/webplugin-alpha.html
platform/chromium/compositing/webgl-loses-compositor-context.html
platform/chromium/compositing/backface-visibility-transformed.html
platform/chromium/compositing/lost-compositor-context-with-rendersurface.html
platform/chromium/compositing/lost-compositor-context.html
platform/chromium/compositing/img-layer-grow.html
platform/chromium/compositing/filters/background-filter-blur-off-axis.html
platform/chromium/compositing/filters/background-filter-blur.html
platform/chromium/compositing/lost-compositor-context-twice.html

[WebKit Review Bot]

Created attachment 149126 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Varun Jain]

(In reply to comment #9)
> Thanks.  I guess I don't really understand why Chromium needs to perform this transformation but other ports don't.  What's different about the Chromium port that causes Chromium (and no other ports) to need to do this?

Hi Adam, Dana..
apologies for the late response (got busy with some other bugs). For mac, this transformation is already handled by mac specific code in webkit2 added in https://bugs.webkit.org/show_bug.cgi?id=67779
For other platforms, I am not sure if this case is handled or not, but to be safe, I am only doing this for chromium.

[Varun Jain]

Created attachment 150233 [details]
Patch

[Varun Jain]

(In reply to comment #10)
> (From update of attachment 149088 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149088&action=review
> 
> > Source/WTF/wtf/Platform.h:1143
> > +/* On Chromium, we apply device scale to all drag images */
> 
> who is "we" in this sentence, maybe it can be more verbose?

Added more explanation. PTAL and let me know if more needs to be added.

> 
> > Source/WebCore/page/DragController.cpp:909
> > +    imgRect.setHeight(imgRect.height() * deviceScaleFactor);
> > +    imgRect.setWidth(imgRect.width() * deviceScaleFactor);
> 
> For non-integer scale factors, is this supposed to truncate? round? expand out? (probably the latter is correct?)
> 
> Maybe you can construct a FloatRect and then use its enclosingIntRect?

Done.

[Dana Jansens]

Comment on attachment 150233 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150233&action=review

> Source/WebCore/page/DragController.cpp:912
> +    return scaleDragImage(dragImage, FloatSize(deviceScaleFactor, deviceScaleFactor));

I took a look at scaleDragImage in DragImageChromiumSkia.cpp and it multiplies and truncates the size back to ints. If we're expanding here, we should ensure it has the same behaviour.

[Dana Jansens]

Comment on attachment 150233 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150233&action=review

>> Source/WebCore/page/DragController.cpp:912
>> +    return scaleDragImage(dragImage, FloatSize(deviceScaleFactor, deviceScaleFactor));
> 
> I took a look at scaleDragImage in DragImageChromiumSkia.cpp and it multiplies and truncates the size back to ints. If we're expanding here, we should ensure it has the same behaviour.

In DragImageChromiumMac.cpp it scales the resulting width/height, to complete the set. We should be consistent everywhere.

[Adam Barth]

I don't think we should be adding an ifdef for this behavior.  As a general rule, we don't make code Chromium-only "just to be safe."  That leads to madness as every port will try to this bug in a different way.  Instead, we should find a general solution.

I don't understand this topic enough to know whether it makes sense for Mac to do this work in a Mac-specific place.  For example, one possible solution is to move that work to a port-independent location.

[Varun Jain]

Created attachment 152895 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 152895 [details]
Patch

Attachment 152895 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13272323

[Build Bot]

Comment on attachment 152895 [details]
Patch

Attachment 152895 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13278332

[Build Bot]

Comment on attachment 152895 [details]
Patch

Attachment 152895 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13277350

[Early Warning System Bot]

Comment on attachment 152895 [details]
Patch

Attachment 152895 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13280327

[Early Warning System Bot]

Comment on attachment 152895 [details]
Patch

Attachment 152895 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13273354

[Varun Jain]

Created attachment 152908 [details]
Patch

[Varun Jain]

(In reply to comment #18)
> I don't think we should be adding an ifdef for this behavior.  As a general rule, we don't make code Chromium-only "just to be safe."  That leads to madness as every port will try to this bug in a different way.  Instead, we should find a general solution.
> 
> I don't understand this topic enough to know whether it makes sense for Mac to do this work in a Mac-specific place.  For example, one possible solution is to move that work to a port-independent location.

Hi Adam, I have modified the patch according to our discussion. PTAL. Heres a summary of modification:

1. DragImageRef which was simply SkBitmap for chromium now also has a resolutionScale that specifies what scale the drag image is at.
2. Before sending the drag image back to the browser, we scale it to match its resolutionScale with deviceScale.
3. NativeImagePtr which, for chromium, is also backed by SkBitmap, now also has resolutionScale.

[WebKit Review Bot]

Comment on attachment 152908 [details]
Patch

Attachment 152908 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13269353

New failing tests:
fast/events/drag-link.html
fast/events/drag-parent-node.html
editing/pasteboard/drop-text-without-selection.html
fast/events/drag-in-frames.html
fast/css/user-drag-none.html
fast/events/content-changed-during-drop.html
editing/pasteboard/files-during-page-drags.html
editing/pasteboard/drop-link.html

[WebKit Review Bot]

Created attachment 152914 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Varun Jain]

Created attachment 152915 [details]
Patch

[WebKit Review Bot]

Comment on attachment 152915 [details]
Patch

Attachment 152915 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13272387

New failing tests:
fast/events/drag-link.html
fast/events/drag-parent-node.html
editing/pasteboard/drop-text-without-selection.html
fast/events/drag-in-frames.html
editing/pasteboard/drop-link.html
fast/events/content-changed-during-drop.html
editing/pasteboard/files-during-page-drags.html
fast/css/user-drag-none.html

[WebKit Review Bot]

Created attachment 152923 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Varun Jain]

Created attachment 153022 [details]
Patch

[Varun Jain]

(In reply to comment #32)
> Created an attachment (id=153022) [details]
> Patch

Resolved tests failures

[Adam Barth]

Comment on attachment 153022 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153022&action=review

Thank you!  This looks much better.

> Source/WebKit/chromium/src/DragClientImpl.cpp:91
> +    // Bring drag image to correct device scale if it is not at it already.

This comment just restates what the code below does.  A more helpful comment would explain when this can happen.  I might just remove the comment.

> Source/WebKit/chromium/src/DragClientImpl.cpp:92
> +    if (dragImage && dragImage->bitmap && m_webView->deviceScaleFactor() != dragImage->resolutionScale) {

Can dragImage ever be null here?

> Source/WebKit/chromium/src/DragClientImpl.cpp:95
> +      ASSERT(dragImage->resolutionScale > 0);
> +      float scale = m_webView->deviceScaleFactor() / dragImage->resolutionScale;
> +      dragImage = scaleDragImage(dragImage, WebCore::FloatSize(scale, scale));

Four-space indent pls.

[Varun Jain]

Created attachment 153031 [details]
Patch

[Varun Jain]

Thanks for the quick review!

(In reply to comment #34)
> (From update of attachment 153022 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153022&action=review
> 
> Thank you!  This looks much better.
> 
> > Source/WebKit/chromium/src/DragClientImpl.cpp:91
> > +    // Bring drag image to correct device scale if it is not at it already.
> 
> This comment just restates what the code below does.  A more helpful comment would explain when this can happen.  I might just remove the comment.

Done.

> 
> > Source/WebKit/chromium/src/DragClientImpl.cpp:92
> > +    if (dragImage && dragImage->bitmap && m_webView->deviceScaleFactor() != dragImage->resolutionScale) {
> 
> Can dragImage ever be null here?

Previous code checked for it. Also, the test failures in my previous patches was due to null dragImage.

> 
> > Source/WebKit/chromium/src/DragClientImpl.cpp:95
> > +      ASSERT(dragImage->resolutionScale > 0);
> > +      float scale = m_webView->deviceScaleFactor() / dragImage->resolutionScale;
> > +      dragImage = scaleDragImage(dragImage, WebCore::FloatSize(scale, scale));
> 
> Four-space indent pls.

Done.

[WebKit Review Bot]

Comment on attachment 153031 [details]
Patch

Clearing flags on attachment: 153031

Committed r122996: <http://trac.webkit.org/changeset/122996>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

This change is causing DragImageTest.NullHandling to crash in test_shell_tests.

[Ryosuke Niwa]

[ RUN      ] DragImageTest.NullHandling
	0   webkit_unit_tests                   0x7ade5f3a 0x0 + 2061393722
	1   webkit_unit_tests                   0x7ae06051 0x0 + 2061525073
	2   libSystem.B.dylib                   0x9662305b _sigtramp + 43
	3   ???                                 0xffffffff 0x0 + 4294967295
	4   webkit_unit_tests                   0x7a99774b 0x0 + 2056877899
	5   webkit_unit_tests                   0x7add1fb0 0x0 + 2061311920
	6   webkit_unit_tests                   0x7adcaee7 0x0 + 2061283047
	7   webkit_unit_tests                   0x7adcb2f4 0x0 + 2061284084
	8   webkit_unit_tests                   0x7adcb6f8 0x0 + 2061285112
	9   webkit_unit_tests                   0x7add0508 0x0 + 2061305096
	10  webkit_unit_tests                   0x7add2510 0x0 + 2061313296
	11  webkit_unit_tests                   0x7add005f 0x0 + 2061303903
	12  webkit_unit_tests                   0x7ae1d673 0x0 + 2061620851
	13  webkit_unit_tests                   0x7a0b3923 0x0 + 2047555875
	14  webkit_unit_tests                   0x7a0b38b6 0x0 + 2047555766
ax: 0, bx: 7a9974d0, cx: 0, dx: 137bb0
di: 7a9974e1, si: 6f027e0, bp: bffff0c8, sp: bffff0c0, ss: 23, flags: 10246
ip: 7b97dca8, cs: 1b, ds: 23, es: 23, fs: 0, gs: f

[Tony Chang]

Test was fixed in http://trac.webkit.org/changeset/123001 .