Bug 125375 – REGRESSION (r160152): Selection drag snapshot doesn't appear or has the wrong content on Retina

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Tim Horton]

On Safari with ToT WebKit:

Steps to Reproduce:

1. Select some text.
2. Drag it.

Expected: a snapshot of the selected content to smoothly move with the cursor, away from the page.

Actual: most of the time, no snapshot. sometimes, a completely wrong snapshot (wrong part of the page, too much content rendered, etc.)

[Brian Burg]

We are looking for confirmation, I am unable to reproduce myself. For me, ToT behavior is identical to WebKit shipped with Safari 7.

[Tim Horton]

(In reply to comment #1)
> We are looking for confirmation, I am unable to reproduce myself. For me, ToT behavior is identical to WebKit shipped with Safari 7.

Simon reproduced as well, earlier.

[Darin Adler]

I think we are going to have to roll the patch out and try again.

[Brian Burg]

Does this reproduce in a nightly as well?

I haven't been able to reproduce on any of my three machines of various OSX versions, using a checkout build nor webkit nightly. Without a test case, stack trace, specific webpage, or something else, I have no way to know whether this is fixed.

[Tim Horton]

I can reproduce with a nightly on Mavericks, but only on Retina hardware (and running that at 1x, everything works fine). Possible something got broken when deviceScaleFactor≠1?

[Brian Burg]

(In reply to comment #5)
> I can reproduce with a nightly on Mavericks, but only on Retina hardware (and running that at 1x, everything works fine). Possible something got broken when deviceScaleFactor≠1?

Okay, that helps a lot. I believe the call to ImageBuffer::create has confused the parameters.

    float deviceScaleFactor = frame.page()->deviceScaleFactor();
    IntRect usedRect(imageRect);
    usedRect.scale(deviceScaleFactor);

    std::unique_ptr<ImageBuffer> buffer = ImageBuffer::create(usedRect.size(), deviceScaleFactor, ColorSpaceDeviceRGB);

It is passing in the computed deviceRect as well as the deviceScaleFactor. Based on other callsites, this should be passing deviceRect and scaleFactor=1, or logicalRect and scaleFactor, but not both.

Could someone do me a favor and change deviceScaleFactor to 1, and see if this is still an issue for retina devices? I can write a test for scaleFactor ≠ 1, but it will have to be tomorrow as I have a huge deadline in 25 hours.

[Darin Adler]

I think we have left this regression in the build too many days. Is it time for a rollout?

[Brian Burg]

(In reply to comment #7)
> I think we have left this regression in the build too many days. Is it time for a rollout?

I am working on a test case and fix as we speak. If it's not fixed by tomorrow, we should roll out.

[Timothy Hatcher]

I am building and checking on a retina machine for Brian.

[Brian Burg]

(In reply to comment #9)
> I am building and checking on a retina machine for Brian.

We have identified several of the root causes and fixed the regressions locally. I'll have a clean patch ready shortly tonight.

[Brian Burg]

Created attachment 219050 [details]
patch

[Darin Adler]

Comment on attachment 219050 [details]
patch

This code worked before refactoring. Why did we have to make all those changes in ImageBuffer::copyImage just to restore behavior to how it was before? The previous patch didn’t have ImageBufferCG.cpp changes.

[Brian Burg]

(In reply to comment #12)
> (From update of attachment 219050 [details])
> This code worked before refactoring. Why did we have to make all those changes in ImageBuffer::copyImage just to restore behavior to how it was before? The previous patch didn’t have ImageBufferCG.cpp changes.

The previous implementation only really worked by accident, because we had erroneously allocated a buffer at 4x in DragImage and incorrectly scaled it down in ImageBuffer::copyImage, but the two bugs cancelled each other out.

[Darin Adler]

Comment on attachment 219050 [details]
patch

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.

[Simon Fraser (smfr)]

Myles should look at the ImageBufferCG.cpp change.

[Brian Burg]

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

[Brian Burg]

Created attachment 219087 [details]
revised patch - needs test on retina

[Darin Adler]

(In reply to comment #16)
> > 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?

Sorry, I wasn’t referring to automated tests, although those would be great!

[Timothy Hatcher]

(In reply to comment #18)
> (In reply to comment #16)
> > > 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?
> 
> Sorry, I wasn’t referring to automated tests, although those would be great!

I was testing the various paths on my retina machine for Brian.

His latest patch (attachment 219087 [details]) fixes the selection and HTML5 dragging cases, but regresses <img> drag — the drag image is half size. We are looking into that.

[Myles C. Maxfield]

Comment on attachment 219087 [details]
revised patch - needs test on retina

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

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:223
> +        FloatSize imageRectInUserBounds = scaleSizeToUserSpace(logicalSize(), m_data.m_backingStoreSize, internalSize());

Good catch! Thanks for fixing this.

[Tim Horton]

Any progress on this?

[Brian Burg]

(In reply to comment #21)
> Any progress on this?

IIRC, the first attachment fixes the regression, but doesn't address the underlying incorrect retina-only scaling code. Before break, Timothy and I got most of the way to a patch that fixes both the regression and the incorrect code, but there was still one case that wasn't fixed yet.

I'll be landing in Cupertino on Monday for orientation and will have a retina macbook. So, we can land the first patch and later undo most of it, or wait until Tuesday and land one fix.

[Blaze Burg]

Created attachment 220668 [details]
v3

[Blaze Burg]

The latest patch up for review fixes all regressions on retina, and also works for non-retina.

[Blaze Burg]

Created attachment 220686 [details]
v3 (resubmit for EWS)

[Myles C. Maxfield]

ImageBufferCG.cpp still looks good to me

[Build Bot]

Comment on attachment 220686 [details]
v3 (resubmit for EWS)

Attachment 220686 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5653974405349376

New failing tests:
media/track/media-element-enqueue-event-crash.html

[Build Bot]

Created attachment 220688 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Darin Adler]

Comment on attachment 220686 [details]
v3 (resubmit for EWS)

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

Looks good. I think we have some opportunity to do some additional drag code refactoring to improve this further in the future, but this seems like a clean step forward.

> Source/WebCore/page/DragController.cpp:832
> +            // Later code expects the drag image to be scaled by device's scale factor.
> +            dragImage = scaleDragImage(dragImage, FloatSize(m_page.deviceScaleFactor(), m_page.deviceScaleFactor()));

My hope is that at some point we can refine further and can eliminate this and move it somewhere more logical. The comment is true, yet a little bit low level, mechanical and confusing.

> Source/WebCore/page/DragController.cpp:874
> +        dragImage = scaleDragImage(dragImage, FloatSize(m_page.deviceScaleFactor(), m_page.deviceScaleFactor()));

Ditto.

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:222
> +        CGContextClipToRect(context.get(), CGRectMake(0, 0, logicalSize().width(), logicalSize().height()));

I think that this:

    CGRectMake(0, 0, logicalSize().width(), logicalSize().height())

is better written as one of these shorter expressions that does not repeat the call to logicalSize():

    FloatRect(FloatPoint(), logicalSize())
    FloatRect(FloatPoint::zero(), logicalSize()

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:223
> +        FloatSize imageRectInUserBounds = scaleSizeToUserSpace(logicalSize(), m_data.m_backingStoreSize, internalSize());

This is a size, not a rect. I think, in fact, it might be better as a rect, one of these:

    FloatRect imageRectInUserSpace = FloatRect(FloatPoint(), scaleSizeToUserSpace(logicalSize(), m_data.m_backingStoreSize, internalSize());
    FloatRect imageRectInUserSpace = FloatRect(FloatPoint::zero(), scaleSizeToUserSpace(logicalSize(), m_data.m_backingStoreSize, internalSize());

or we could rename it imageSizeInUserSpace.

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:224
> +        CGContextDrawImage(context.get(), CGRectMake(0, 0, imageRectInUserBounds.width(), imageRectInUserBounds.height()), image.get());

I think that this:

    CGRectMake(0, 0, imageRectInUserBounds.width(), imageRectInUserBounds.height())

is better written as one of these shorter expressions:

    imageRectInUserSpace
    FloatRect(FloatPoint(), imageSizeInUserSpace)
    FloatRect(FloatPoint::zero(), imageSizeInUserSpace)

[Blaze Burg]

Created attachment 220758 [details]
v3 with style fixes

[WebKit Commit Bot]

Comment on attachment 220758 [details]
v3 with style fixes

Clearing flags on attachment: 220758

Committed r161577: <http://trac.webkit.org/changeset/161577>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.