Bug 125375 - REGRESSION (r160152): Selection drag snapshot doesn't appear or has the wrong content on Retina
Summary: REGRESSION (r160152): Selection drag snapshot doesn't appear or has the wrong...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Brian Burg
URL:
Keywords:
Depends on: 126661
Blocks:
  Show dependency treegraph
 
Reported: 2013-12-06 18:08 PST by Tim Horton
Modified: 2014-01-09 13:17 PST (History)
14 users (show)

See Also:


Attachments
patch (14.54 KB, patch)
2013-12-12 00:15 PST, Brian Burg
no flags Details | Formatted Diff | Diff
revised patch - needs test on retina (7.84 KB, patch)
2013-12-12 09:01 PST, Brian Burg
no flags Details | Formatted Diff | Diff
v3 (9.30 KB, patch)
2014-01-08 14:48 PST, Brian Burg
no flags Details | Formatted Diff | Diff
v3 (resubmit for EWS) (9.30 KB, patch)
2014-01-08 18:27 PST, Brian Burg
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (463.13 KB, application/zip)
2014-01-08 18:55 PST, Build Bot
no flags Details
v3 with style fixes (10.36 KB, patch)
2014-01-09 12:29 PST, Brian Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2013-12-06 18:08:43 PST
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.)
Comment 1 Brian Burg 2013-12-06 19:06:34 PST
We are looking for confirmation, I am unable to reproduce myself. For me, ToT behavior is identical to WebKit shipped with Safari 7.
Comment 2 Tim Horton 2013-12-06 19:10:24 PST
(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.
Comment 3 Darin Adler 2013-12-09 08:52:11 PST
I think we are going to have to roll the patch out and try again.
Comment 4 Brian Burg 2013-12-09 10:16:25 PST
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.
Comment 5 Tim Horton 2013-12-09 11:02:15 PST
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?
Comment 6 Brian Burg 2013-12-09 11:12:14 PST
(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.
Comment 7 Darin Adler 2013-12-11 14:20:47 PST
I think we have left this regression in the build too many days. Is it time for a rollout?
Comment 8 Brian Burg 2013-12-11 14:22:03 PST
(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.
Comment 9 Timothy Hatcher 2013-12-11 15:09:54 PST
I am building and checking on a retina machine for Brian.
Comment 10 Brian Burg 2013-12-11 21:52:17 PST
(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.
Comment 11 Brian Burg 2013-12-12 00:15:21 PST
Created attachment 219050 [details]
patch
Comment 12 Darin Adler 2013-12-12 00:18:36 PST
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.
Comment 13 Brian Burg 2013-12-12 00:27:29 PST
(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.
Comment 14 Darin Adler 2013-12-12 00:40:48 PST
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.
Comment 15 Simon Fraser (smfr) 2013-12-12 08:23:54 PST
Myles should look at the ImageBufferCG.cpp change.
Comment 16 Brian Burg 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.
Comment 17 Brian Burg 2013-12-12 09:01:23 PST
Created attachment 219087 [details]
revised patch - needs test on retina
Comment 18 Darin Adler 2013-12-12 10:14:18 PST
(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!
Comment 19 Timothy Hatcher 2013-12-12 11:07:07 PST
(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.
Comment 20 Myles C. Maxfield 2013-12-12 11:51:52 PST
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.
Comment 21 Tim Horton 2014-01-02 03:46:41 PST
Any progress on this?
Comment 22 Brian Burg 2014-01-02 09:19:07 PST
(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.
Comment 23 Brian Burg 2014-01-08 14:48:35 PST
Created attachment 220668 [details]
v3
Comment 24 Brian Burg 2014-01-08 16:20:26 PST
The latest patch up for review fixes all regressions on retina, and also works for non-retina.
Comment 25 Brian Burg 2014-01-08 18:27:59 PST
Created attachment 220686 [details]
v3 (resubmit for EWS)
Comment 26 Myles C. Maxfield 2014-01-08 18:33:01 PST
ImageBufferCG.cpp still looks good to me
Comment 27 Build Bot 2014-01-08 18:55:23 PST
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
Comment 28 Build Bot 2014-01-08 18:55:27 PST
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
Comment 29 Darin Adler 2014-01-09 09:07:24 PST
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)
Comment 30 Brian Burg 2014-01-09 12:29:23 PST
Created attachment 220758 [details]
v3 with style fixes
Comment 31 WebKit Commit Bot 2014-01-09 13:17:54 PST
Comment on attachment 220758 [details]
v3 with style fixes

Clearing flags on attachment: 220758

Committed r161577: <http://trac.webkit.org/changeset/161577>
Comment 32 WebKit Commit Bot 2014-01-09 13:17:59 PST
All reviewed patches have been landed.  Closing bug.