[webkit-reviews] review granted: [Bug 125375] REGRESSION (r160152): Selection drag snapshot doesn't appear or has the wrong content on Retina : [Attachment 219050] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Brian Burg <burg at cs.washington.edu>'s
request for review:
Bug 125375: REGRESSION (r160152): Selection drag snapshot doesn't appear or has
the wrong content on Retina
https://bugs.webkit.org/show_bug.cgi?id=125375

Attachment 219050: patch
https://bugs.webkit.org/attachment.cgi?id=219050&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.