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.)
We are looking for confirmation, I am unable to reproduce myself. For me, ToT behavior is identical to WebKit shipped with Safari 7.
(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.
I think we are going to have to roll the patch out and try again.
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.
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?
(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.
I think we have left this regression in the build too many days. Is it time for a rollout?
(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.
I am building and checking on a retina machine for Brian.
(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.
Created attachment 219050 [details] patch
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.
(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 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.
Myles should look at the ImageBufferCG.cpp change.
> 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.
Created attachment 219087 [details] revised patch - needs test on retina
(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!
(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 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.
Any progress on this?
(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.
Created attachment 220668 [details] v3
The latest patch up for review fixes all regressions on retina, and also works for non-retina.
Created attachment 220686 [details] v3 (resubmit for EWS)
ImageBufferCG.cpp still looks good to me
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
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 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)
Created attachment 220758 [details] v3 with style fixes
Comment on attachment 220758 [details] v3 with style fixes Clearing flags on attachment: 220758 Committed r161577: <http://trac.webkit.org/changeset/161577>
All reviewed patches have been landed. Closing bug.