[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





--- Comment #16 from Brian Burg <burg at cs.washington.edu>  2013-12-12 08:53:40 PST ---
> 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.

That is a good point. After looking more, it seems we hand off an NSImage to the OS, which it knows how to scale based on where it is being shown.

> > 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.

You are right- it's not needed. It seems this is counteracting a wrong scaling operation in the opposite direction in WebDragClientMac. I'll try removing that instead, since it now receives an unscaled image.

> 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.

Good point. In the new patch, this is just delegated to WKView and the OS as before, but without unnecessary scaling by 1/2x then 2x along the way.

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

To my knowledge, all of the drag-related test cases in TestWebKitAPI are reftests and wouldn't be able to detect bad scaling behavior if all the snapshot methods do it incorrectly the same way. Are there any examples of deviceScaleFactor tests that check for similar regressions?

> 
> > 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.

Ok, removed.

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