[Webkit-unassigned] [Bug 125375] REGRESSION (r160152): Selection drag snapshot doesn't appear or has the wrong content on Retina

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #219050|review?                     |review+
               Flag|                            |




--- Comment #14 from Darin Adler <darin at apple.com>  2013-12-12 00:40:48 PST ---
(From update of attachment 219050)
View in context: https://bugs.webkit.org/attachment.cgi?id=219050&action=review

> Source/WebCore/dom/Clipboard.cpp:277
>      if (m_dragImage)
> -        return createDragImageFromImage(m_dragImage->image(), ImageOrientationDescription());
> +        // FIXME: use the Page's deviceScaleFactor if it could change for !PLATFORM(MAC).
> +        return createDragImageFromImage(m_dragImage->image(), ImageOrientationDescription(), 1);

Formatting mistakes here. Multiple line if body needs braces in WebKit coding style even if the new line is a comment. Comment uses sentence style so it should be “Use” rather than “use”.

I also think that “if it could change for !PLATFORM(MAC)” is too cryptic.

Overall I’m not sure the FIXME is good enough to keep. Is this really the one place that we are hard-coding a device scale factor of 1 for non-Mac platforms? How useful is this comment, really.

If I was including the FIXME, my wording would be:

    // FIXME: Need to pass appropriate scale factor here. For now hardcoded to 1.

And I’m not sure that Page’s deviceScaleFactor is correct. After all, a drag image is used for dragging feedback, and on the Mac at least, that drag can go across displays and so picking the scale factor of the source webpage is not necessarily right.

> Source/WebCore/page/DragController.cpp:867
> -        && (dragImage = createDragImageFromImage(image, element.renderer() ? orientationDescription : ImageOrientationDescription()))) {
> +        && (dragImage = createDragImageFromImage(image, element.renderer() ? orientationDescription : ImageOrientationDescription(), m_page.deviceScaleFactor()))) {

Why is this device scale factor argument needed now? It wasn’t needed before the patch that caused the regression.

I’m not sure that passing in the scale factor of the source window is correct. On a Mac where we had a window on a 1X screen but also had a 2X screen, I think we might want a 2X image.

How did we test this code path? What kinds of tests fail when we pass 1 here instead of the device scale factor?

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:227
> +        ASSERT(CGImageGetWidth(image.get()) == static_cast<size_t>(m_logicalSize.width()));
> +        ASSERT(CGImageGetHeight(image.get()) == static_cast<size_t>(m_logicalSize.height()));

Not sure these assertions do any good now that they are only in one side of the if statement. In this code snippet, these assertions say little, because these are the values we passed into CGBitmapContextCreate above, so of course the assertion is true. I’d consider leaving them out.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.