|
Description
Said Abou-Hallawa
2016-11-22 23:06:08 PST
Created attachment 295353 [details]
Patch
Created attachment 295354 [details]
Patch
Created attachment 295355 [details]
Patch
Created attachment 295356 [details]
Patch
Comment on attachment 295356 [details] Patch Attachment 295356 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2559609 Number of test failures exceeded the failure limit. Created attachment 295358 [details]
Archive of layout-test-results from ews103 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 295356 [details] Patch Attachment 295356 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2559615 Number of test failures exceeded the failure limit. Created attachment 295359 [details]
Archive of layout-test-results from ews116 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 295356 [details] Patch Attachment 295356 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2559636 New failing tests: fast/images/image-subsampling.html fast/flexbox/image-percent-max-height.html canvas/philip/tests/2d.drawImage.9arg.sourcepos.html platform/ios-simulator/ios/fast/canvas/image_subSampling_scale.html imported/w3c/canvas/2d.drawImage.9arg.sourcesize.html canvas/philip/tests/2d.drawImage.negativedest.html compositing/backgrounds/fixed-background-on-descendant.html fast/multicol/newmulticol/compare-with-old-impl/shrink-to-column-height-for-pagination.html imported/w3c/canvas/2d.drawImage.negativedir.html imported/w3c/canvas/2d.drawImage.negativedest.html fast/canvas/canvas-drawImage-shadow.html imported/w3c/canvas/2d.drawImage.9arg.sourcepos.html canvas/philip/tests/2d.drawImage.negativesource.html fast/css/object-fit/object-fit-canvas.html fast/flexbox/aspect-ratio-intrinsic-adjust.html compositing/backgrounds/fixed-backgrounds.html imported/w3c/canvas/2d.drawImage.negativesource.html fast/canvas/image-potential-subsample.html canvas/philip/tests/2d.drawImage.negativeSourceHeight.html canvas/philip/tests/2d.drawImage.negativedir.html Created attachment 295360 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 295356 [details] Patch Attachment 295356 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2559792 New failing tests: fast/images/image-subsampling.html canvas/philip/tests/2d.drawImage.9arg.sourcepos.html canvas/philip/tests/2d.drawImage.negativedest.html imported/w3c/canvas/2d.pattern.paint.repeat.coord2.html fast/flexbox/image-percent-max-height.html fast/flexbox/aspect-ratio-intrinsic-adjust.html webgl/1.0.2/conformance/canvas/to-data-url-test.html imported/w3c/canvas/2d.drawImage.negativedest.html imported/w3c/canvas/2d.drawImage.negativedir.html imported/w3c/canvas/2d.drawImage.9arg.sourcepos.html fast/css/object-fit/object-fit-shrink.html canvas/philip/tests/2d.pattern.paint.repeat.coord1.html fast/borders/border-painting-correctness-dashed.html canvas/philip/tests/2d.drawImage.negativesource.html imported/w3c/canvas/2d.drawImage.negativesource.html imported/w3c/canvas/2d.drawImage.9arg.sourcesize.html fast/canvas/canvas-blending-image-over-color.html fast/multicol/newmulticol/compare-with-old-impl/shrink-to-column-height-for-pagination.html imported/w3c/canvas/2d.pattern.paint.repeat.coord1.html fast/canvas/canvas-drawImage-shadow.html fast/borders/border-painting-correctness-dotted.html canvas/philip/tests/2d.pattern.paint.repeat.coord3.html canvas/philip/tests/2d.pattern.paint.repeat.coord2.html imported/w3c/canvas/2d.pattern.paint.repeat.coord3.html canvas/philip/tests/2d.drawImage.9arg.sourcesize.html canvas/philip/tests/2d.drawImage.negativeSourceHeight.html canvas/philip/tests/2d.drawImage.negativedir.html Created attachment 295361 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 295381 [details]
Patch
Created attachment 295382 [details]
Patch
Comment on attachment 295382 [details] Patch Attachment 295382 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2562528 New failing tests: imported/blink/fast/frames/navigation-in-pagehide.html svg/custom/anchor-on-use.svg Created attachment 295385 [details]
Archive of layout-test-results from ews102 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 295386 [details]
Patch
Comment on attachment 295386 [details] Patch Attachment 295386 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2562748 New failing tests: imported/blink/fast/frames/navigation-in-pagehide.html fast/css/object-fit/object-fit-embed.html svg/custom/anchor-on-use.svg Created attachment 295388 [details]
Archive of layout-test-results from ews102 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 295386 [details] Patch Attachment 295386 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2562769 New failing tests: imported/blink/fast/frames/navigation-in-pagehide.html Created attachment 295389 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 295386 [details] Patch Attachment 295386 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2562768 New failing tests: imported/blink/fast/frames/navigation-in-pagehide.html fast/css/object-fit/object-fit-embed.html svg/custom/anchor-on-use.svg Created attachment 295390 [details]
Archive of layout-test-results from ews117 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 295386 [details] Patch Attachment 295386 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2562780 New failing tests: imported/blink/fast/frames/navigation-in-pagehide.html Created attachment 295391 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 295398 [details]
Patch
Comment on attachment 295398 [details] Patch Attachment 295398 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2564490 New failing tests: fast/css/object-fit/object-fit-embed.html Created attachment 295401 [details]
Archive of layout-test-results from ews102 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 295398 [details] Patch Attachment 295398 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2564506 New failing tests: fast/css/object-fit/object-fit-embed.html Created attachment 295403 [details]
Archive of layout-test-results from ews115 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 295460 [details]
Patch
Created attachment 295464 [details]
Patch
Created attachment 295465 [details]
Patch
Comment on attachment 295465 [details] Patch Attachment 295465 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2579935 New failing tests: fast/css/object-fit/object-fit-embed.html Created attachment 295468 [details]
Archive of layout-test-results from ews103 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 295465 [details] Patch Attachment 295465 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2579961 New failing tests: fast/css/object-fit/object-fit-embed.html Created attachment 295470 [details]
Archive of layout-test-results from ews114 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 295478 [details]
Patch
Comment on attachment 295478 [details] Patch Attachment 295478 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2581106 New failing tests: fast/css/object-fit/object-fit-embed.html transitions/default-timing-function.html Created attachment 295479 [details]
Archive of layout-test-results from ews102 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 295478 [details] Patch Attachment 295478 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2581110 New failing tests: fast/css/object-fit/object-fit-embed.html Created attachment 295480 [details]
Archive of layout-test-results from ews117 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 295481 [details]
Patch
Created attachment 295483 [details]
Patch
Created attachment 295503 [details]
Patch
Created attachment 302351 [details]
Patch
Comment on attachment 302351 [details] Patch Attachment 302351 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3169736 New failing tests: compositing/backgrounds/fixed-background-on-descendant.html Created attachment 302355 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 302435 [details]
Patch
Comment on attachment 302435 [details] Patch Attachment 302435 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3174868 New failing tests: fast/dom/timer-throttling-hidden-page.html Created attachment 302449 [details]
Archive of layout-test-results from ews115 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
(In reply to comment #50) > Comment on attachment 302435 [details] > Patch > > Attachment 302435 [details] did not pass mac-debug-ews (mac): > Output: http://webkit-queues.webkit.org/results/3174868 > > New failing tests: > fast/dom/timer-throttling-hidden-page.html Likely a flake, this is a test I have landed today. Created attachment 302494 [details]
Patch
Comment on attachment 302494 [details] Patch Attachment 302494 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3177475 New failing tests: fast/dom/timer-throttling-hidden-page.html Created attachment 302501 [details]
Archive of layout-test-results from ews115 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
(In reply to comment #52) > (In reply to comment #50) > > Comment on attachment 302435 [details] > > Patch > > > > Attachment 302435 [details] did not pass mac-debug-ews (mac): > > Output: http://webkit-queues.webkit.org/results/3174868 > > > > New failing tests: > > fast/dom/timer-throttling-hidden-page.html > > Likely a flake, this is a test I have landed today. Shouldn't this test be marked as a flaky test? https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fdom%2Ftimer-throttling-hidden-page.html Comment on attachment 302494 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302494&action=review It's really unfortunate that we pass MaxPixelSize down through all these functions. Can we pass down something like a std::optional<IntSize>sizeForDraw or something instead? > Source/WebCore/ChangeLog:20 > + If image source size is large (e.g. 3000x3000 pixels) and the size of the > + destination rectangle is small, CGImageSourceCreateThumbnailAtIndex() is > + slower than CGImageSourceCreateImageAtIndex() in decoding this image. To > + overcome this problem, the entry (kCGImageSourceThumbnailMaxPixelSize, > + max(destinationRect.width, destinationRect.height)) is added to the options > + dictionary when calling CGImageSourceCreateThumbnailAtIndex(). This will > + avoid copying a large block of memory for the unscaled bitmap image. This seems like a different patch. I think you should split this into 2. > Source/WebCore/loader/cache/CachedImage.h:129 > + URL sourceUrl() const override { return m_cachedImages[0]->url(); } Is m_cachedImages guaranteed to have at least one entry? > Source/WebCore/page/FrameView.h:278 > + void requestAsyncDecodingForImagesInRectIncludingSubframes(const IntRect&); This needs to be more specific about what coordinate system 'rect' is in. Is it document coords, or view coords? Does it change with zooming? > Source/WebCore/page/FrameView.h:653 > + void requestAsyncDecodingForImagesInRect(const IntRect&); Ditto. > Source/WebCore/platform/graphics/BitmapImage.cpp:246 > +bool BitmapImage::isAsyncDecodingRequired() Can this be const? > Source/WebCore/platform/graphics/BitmapImage.h:136 > + String sourceURL() const { return imageObserver() ? imageObserver()->sourceUrl().string() : ""; } More efficient to use emptyString() than "". > Source/WebCore/platform/graphics/BitmapImage.h:210 > + MaxPixelSize m_currentMaxPixelSize { MaxPixelSizeNative }; It's not clear what this means. What is the context of "current"? What does "max pixel size" mean for a BitmapImage? > Source/WebCore/platform/graphics/ImageFrame.h:76 > +typedef int MaxPixelSize; You plumb MaxPixelSize through a lot of functions, and it's hard to know what it means. > Source/WebCore/platform/graphics/ImageFrame.h:79 > + MaxPixelSizeUndefined = -1, We try to avoid magic -1 numbers. Can we use std::optional instead? Comment on attachment 302494 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302494&action=review >> Source/WebCore/ChangeLog:20 >> + avoid copying a large block of memory for the unscaled bitmap image. > > This seems like a different patch. I think you should split this into 2. Filed https://bugs.webkit.org/show_bug.cgi?id=168814. *** Bug 161705 has been marked as a duplicate of this bug. *** Created attachment 303555 [details]
Patch
Comment on attachment 303555 [details] Patch This patch include the patch of https://bugs.webkit.org/show_bug.cgi?id=168814. I just want to make sure the test are passing with the combined patch. Comment on attachment 303555 [details] Patch Attachment 303555 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3254591 New failing tests: fast/images/reset-image-animation.html Created attachment 303569 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 303555 [details] Patch Attachment 303555 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3254594 New failing tests: tables/mozilla/bugs/bug4527.html fast/images/reset-image-animation.html Created attachment 303572 [details]
Archive of layout-test-results from ews114 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 303555 [details] Patch Attachment 303555 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3254626 New failing tests: fast/images/reset-image-animation.html Created attachment 303573 [details]
Archive of layout-test-results from ews103 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 303555 [details] Patch Attachment 303555 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3254615 New failing tests: fast/images/reset-image-animation.html Created attachment 303576 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 303596 [details]
Patch
Created attachment 303597 [details]
Patch for 165039 separate from 168814
Created attachment 303687 [details]
Patch
Created attachment 303690 [details]
Patch for 165039 separate from 168814
This patch gets rid of using descendantsOfType<RenderImage>() in RenderView::requestAsyncDecodingForImagesInDocumentRect(). Instead it uses a HashSet of RenderElement which is built by the HTMLImageElement.
I am working on a test. But I have to implement the ondecode event instead of using a timer. If it turns out implementing the ondecode event is very involving, I will fallback to using the timer. Comment on attachment 303690 [details] Patch for 165039 separate from 168814 View in context: https://bugs.webkit.org/attachment.cgi?id=303690&action=review > Source/WebCore/platform/graphics/BitmapImage.cpp:178 > + if (!frameHasDecodedNativeImage(m_currentFrame)) > + return; Don't we want to paint the yellow debug color here too? > Source/WebCore/platform/graphics/BitmapImage.cpp:409 > + imageObserver()->changedInRect(this, nullptr); When is newFrameNativeImageAvailableAtIndex() called? If it's during painting, the repaint triggered by changedInRect() will get dropped on the floor. > Source/WebCore/platform/graphics/BitmapImage.cpp:422 > + if (m_source.requestFrameAsyncDecodingAtIndex(0, m_currentSubsamplingLevel, sizeForDrawing)) > + LOG(Images, "BitmapImage::%s - %p - url: %s", __FUNCTION__, this, sourceURL().characters8()); I would prefer not doing work inside the if (), the LOGging here too. > Source/WebCore/rendering/RenderElement.cpp:1440 > + LayoutRect backgroundPaintingRect = backgroundIsPaintedByRoot ? view().backgroundRect() : absoluteClippedOverflowRect(); absoluteClippedOverflowRect is expensive. We should keep an eye out for this being a perf bottleneck. > Source/WebCore/rendering/RenderElement.cpp:1506 > +void RenderElement::unregisterForAsyncImageDecodingCallback() It makes me nervous that we rely on HTMLImageElement() to call this to get unregistered from the RenderView. If there's any code path where that call doesn't happen, then we have a security bug. I would prefer any RenderElement that was registered is able to unregister itself on teardown. > Source/WebCore/rendering/RenderElement.h:140 > + bool intersects(const IntRect&) const; This should be called intersectsAbsoluteRect or something that clarifies the coordinate system of the rect. > Source/WebCore/rendering/RenderView.cpp:1430 > + if (!m_largeImageRenderers.contains(&renderer)) > + m_largeImageRenderers.add(&renderer); Don't call contains before add. That's two hash lookups instead of one. > Source/WebCore/rendering/RenderView.cpp:1436 > + if (m_largeImageRenderers.contains(&renderer)) > + m_largeImageRenderers.remove(&renderer); Don't call contains just to remove! that's two hash lookups instead of one. > Source/WebCore/rendering/RenderView.h:394 > + HashSet<RenderElement*> m_largeImageRenderers; The same "m_largeImageRenderers" should match the function names "registerForAsyncImageDecodingCallback", so this should be m_asyncDecodingImageRenderers. > Source/WebCore/testing/Internals.cpp:478 > + if (InternalSettings* settings = this->settings()) > + settings->setLargeImageAsyncDecodingEnabled(false); That's not the correct way to disable a setting for testing. You should do it via code in DRT/WTR which changes the WK1 or WK2 pref. Created attachment 303772 [details]
Patch
Created attachment 303814 [details]
Patch
Created attachment 303849 [details]
Patch
Comment on attachment 303690 [details] Patch for 165039 separate from 168814 View in context: https://bugs.webkit.org/attachment.cgi?id=303690&action=review >> Source/WebCore/platform/graphics/BitmapImage.cpp:178 >> + return; > > Don't we want to paint the yellow debug color here too? Yes. Fixed. >> Source/WebCore/platform/graphics/BitmapImage.cpp:409 >> + imageObserver()->changedInRect(this, nullptr); > > When is newFrameNativeImageAvailableAtIndex() called? If it's during painting, the repaint triggered by changedInRect() will get dropped on the floor. newFrameNativeImageAvailableAtIndex() is called from the decoding thread via callOnMainThread(). So missing a repaint should not happen. >> Source/WebCore/platform/graphics/BitmapImage.cpp:422 >> + LOG(Images, "BitmapImage::%s - %p - url: %s", __FUNCTION__, this, sourceURL().characters8()); > > I would prefer not doing work inside the if (), the LOGging here too. Done. >> Source/WebCore/rendering/RenderElement.cpp:1506 >> +void RenderElement::unregisterForAsyncImageDecodingCallback() > > It makes me nervous that we rely on HTMLImageElement() to call this to get unregistered from the RenderView. If there's any code path where that call doesn't happen, then we have a security bug. I would prefer any RenderElement that was registered is able to unregister itself on teardown. Done. I added a call to unregisterForAsyncImageDecodingCallback(*this) in the destructor of RenderElement. >> Source/WebCore/rendering/RenderElement.h:140 >> + bool intersects(const IntRect&) const; > > This should be called intersectsAbsoluteRect or something that clarifies the coordinate system of the rect. Done. Function is renamed. >> Source/WebCore/rendering/RenderView.cpp:1430 >> + m_largeImageRenderers.add(&renderer); > > Don't call contains before add. That's two hash lookups instead of one. Done. Instead I added isRegisteredForAsyncImageDecodingCallback() to RenderObject(). >> Source/WebCore/rendering/RenderView.cpp:1436 >> + m_largeImageRenderers.remove(&renderer); > > Don't call contains just to remove! that's two hash lookups instead of one. Done. >> Source/WebCore/rendering/RenderView.h:394 >> + HashSet<RenderElement*> m_largeImageRenderers; > > The same "m_largeImageRenderers" should match the function names "registerForAsyncImageDecodingCallback", so this should be m_asyncDecodingImageRenderers. Done. Member was renamed. >> Source/WebCore/testing/Internals.cpp:478 >> + settings->setLargeImageAsyncDecodingEnabled(false); > > That's not the correct way to disable a setting for testing. You should do it via code in DRT/WTR which changes the WK1 or WK2 pref. Done. DRT and WTR was changed to disable the large image decoding. Comment on attachment 303849 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303849&action=review > Source/WebCore/page/FrameView.cpp:1063 > + requestAsyncDecodingForImagesInAbsoluteRectIncludingSubframes(tiledBacking->tileCoverageRect()); Please fix the comment in TiledBacking that says: // Exposed for testing virtual IntRect tileCoverageRect() const = 0; > Source/WebCore/platform/graphics/BitmapImage.cpp:420 > + LOG(Images, "BitmapImage::%s - %p - url: %s [earlyFrameCount = %ld nextFrame = %ld]", __FUNCTION__, this, sourceURL().characters8(), ++m_earlyFrameCount, index); Better to use string.utf8().data(). > Source/WebCore/platform/graphics/BitmapImage.cpp:444 > + LOG(Images, "BitmapImage::%s - %p - url: %s", __FUNCTION__, this, sourceURL().characters8()); Better to use string.utf8().data(). > Source/WebCore/rendering/RenderView.cpp:1451 > + float scale = frame().frameScaleFactor() * frame().pageZoomFactor() * document().deviceScaleFactor(); > + if (frame().settings().delegatesPageScaling()) > + scale *= frame().page()->pageScaleFactor(); frameScaleFactor() IS pageScaleFactor() if !delegatesPageScaling, so this is super confusing. I think you just want .page()->pageScaleFactor() * frame().pageZoomFactor() * document().deviceScaleFactor(); > Source/WebCore/rendering/RenderView.cpp:1452 > + image->image()->requestAsyncDecoding(expandedIntSize(renderer->objectBoundingBox().size() * scale)); objectBoundingBox isn't the right box, and may be slightly different from the rendered size, leading to slight resizing artifacts. You need to use the rect that RenderImage::paintReplaced() uses. In addition, you need to file a bug to track taking ancestor transforms into account, including ancestor SVG transforms. I really think we should just delay this request until paint time to avoid having to do all the math again. I don't think we'll get it right doing it here. Created attachment 303870 [details]
Patch
Comment on attachment 303849 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303849&action=review >> Source/WebCore/platform/graphics/BitmapImage.cpp:420 >> + LOG(Images, "BitmapImage::%s - %p - url: %s [earlyFrameCount = %ld nextFrame = %ld]", __FUNCTION__, this, sourceURL().characters8(), ++m_earlyFrameCount, index); > > Better to use string.utf8().data(). Done. >> Source/WebCore/platform/graphics/BitmapImage.cpp:444 >> + LOG(Images, "BitmapImage::%s - %p - url: %s", __FUNCTION__, this, sourceURL().characters8()); > > Better to use string.utf8().data(). Done. >> Source/WebCore/rendering/RenderView.cpp:1451 >> + scale *= frame().page()->pageScaleFactor(); > > frameScaleFactor() IS pageScaleFactor() if !delegatesPageScaling, so this is super confusing. I think you just want .page()->pageScaleFactor() * frame().pageZoomFactor() * document().deviceScaleFactor(); Done. >> Source/WebCore/rendering/RenderView.cpp:1452 >> + image->image()->requestAsyncDecoding(expandedIntSize(renderer->objectBoundingBox().size() * scale)); > > objectBoundingBox isn't the right box, and may be slightly different from the rendered size, leading to slight resizing artifacts. You need to use the rect that RenderImage::paintReplaced() uses. > > In addition, you need to file a bug to track taking ancestor transforms into account, including ancestor SVG transforms. > > I really think we should just delay this request until paint time to avoid having to do all the math again. I don't think we'll get it right doing it here. Filed https://bugs.webkit.org/show_bug.cgi?id=169396. Comment on attachment 303870 [details] Patch Clearing flags on attachment: 303870 Committed r213618: <http://trac.webkit.org/changeset/213618> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 169475 *** Bug 169473 has been marked as a duplicate of this bug. *** Created attachment 304133 [details]
Patch
Comment on attachment 304133 [details] Patch Attachment 304133 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3289079 New failing tests: svg/custom/anchor-on-use.svg Created attachment 304139 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 304133 [details] Patch Attachment 304133 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3289099 New failing tests: svg/custom/anchor-on-use.svg Created attachment 304140 [details]
Archive of layout-test-results from ews102 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 304133 [details] Patch Attachment 304133 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3289102 New failing tests: svg/custom/anchor-on-use.svg Created attachment 304141 [details]
Archive of layout-test-results from ews113 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 304143 [details]
Patch
Created attachment 304164 [details]
Patch
Comment on attachment 304164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=304164&action=review We need to test that the right thing happens for a directly composited image, but otherwise this looks OK. > Source/WebCore/platform/graphics/BitmapImage.cpp:166 > + m_sizeForDrawing = enclosingIntRect(context.getCTM().mapRect(destRect)).size(); I trust this works for retina, page zoom etc? > Source/WebCore/platform/graphics/BitmapImage.cpp:179 > + if (isLargeImageAsyncDecodingRequired()) { Not new to this patch, but the "required" sounds too strong here. Would read better as "shouldUseAsyncDecoding" or something. > Source/WebCore/platform/graphics/BitmapImage.cpp:435 > + if (m_needsRepaint) { I'm not convinced of the necessity of having the m_needsRepaint member. When would newFrameNativeImageAvailableAtIndex() get called when you do *not* want to call changedInRect? > Source/WebCore/platform/graphics/BitmapImage.cpp:440 > + // Keep the number of decoding threads under control. This seems a bit vague. Maybe just remove the comment. > Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:384 > + // CGImageSourceCreateThumbnailAtIndex() creates a bitmap context with the image native size > + // regardless of the subsamplingLevel unless kCGImageSourceSubsampleFactor is passed. Getting > + // frameSizeAtIndex(index, subsamplingLevel) will return irrelevant size to the size of image > + // that is going to be created so get frameSizeAtIndex() for SubsamplingLevel::Default and > + // compare it with sizeForDrawing. I'm having a hard time understanding this comment. First, CGImageSourceCreateThumbnailAtIndex() doesn't crate a bitmap context, it creates a CGImageRef, and it sounds like correct behavior for it to use the native image size unless you specify kCGImageSourceSubsampleFactor. Does passing SubsamplingLevel::Default here just ensure that *our* code behaves as we want? Created attachment 304176 [details]
Patch
Comment on attachment 304164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=304164&action=review >> Source/WebCore/platform/graphics/BitmapImage.cpp:166 >> + m_sizeForDrawing = enclosingIntRect(context.getCTM().mapRect(destRect)).size(); > > I trust this works for retina, page zoom etc? Yes this works with page zoom and view zoom correctly. >> Source/WebCore/platform/graphics/BitmapImage.cpp:179 >> + if (isLargeImageAsyncDecodingRequired()) { > > Not new to this patch, but the "required" sounds too strong here. Would read better as "shouldUseAsyncDecoding" or something. Done. Functions were renamed. >> Source/WebCore/platform/graphics/BitmapImage.cpp:435 >> + if (m_needsRepaint) { > > I'm not convinced of the necessity of having the m_needsRepaint member. When would newFrameNativeImageAvailableAtIndex() get called when you do *not* want to call changedInRect? Done. You are right m_needsRepaint is not needed. >> Source/WebCore/platform/graphics/BitmapImage.cpp:440 >> + // Keep the number of decoding threads under control. > > This seems a bit vague. Maybe just remove the comment. The comment was removed. >> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:384 >> + // compare it with sizeForDrawing. > > I'm having a hard time understanding this comment. First, CGImageSourceCreateThumbnailAtIndex() doesn't crate a bitmap context, it creates a CGImageRef, and it sounds like correct behavior for it to use the native image size unless you specify kCGImageSourceSubsampleFactor. Does passing SubsamplingLevel::Default here just ensure that *our* code behaves as we want? I changed the comment a little. What I meant here is the following: CGImageSourceCreateThumbnailAtIndex() calls CGBitmapContextCreate() to create a CGContext with the image native size regardless of the subsamplingLevel unless kCGImageSourceSubsampleFactor is passed. It then calls CGBitmapContextCreateImage() to get the frame CGImage. Here we are trying to see which size is smaller: the image native size or the sizeForDrawing(). Assuming that frameSizeAtIndex(index, subsamplingLevel) will return the size which CGBitmapContextCreate() will be called with was wrong. To fix this issue we need to get frameSizeAtIndex() for SubsamplingLevel::Default and compare it the sizeForDrawing. (In reply to comment #98) > Comment on attachment 304164 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=304164&action=review > > We need to test that the right thing happens for a directly composited > image, but otherwise this looks OK. > Did you mean something like this? <style> .transformed { transform: scale(2); transform-origin: top left; width: 400px; height: 400px; } </style> <body> <div class="transformed"> <img src="image.jpg"> </div> </body> I tested this case and the quality of the image is correct. (In reply to comment #101) > (In reply to comment #98) > > Comment on attachment 304164 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=304164&action=review > > > > We need to test that the right thing happens for a directly composited > > image, but otherwise this looks OK. > > > > Did you mean something like this? > > <style> > .transformed { > transform: scale(2); > transform-origin: top left; > width: 400px; > height: 400px; > } > </style> > <body> > <div class="transformed"> > <img src="image.jpg"> > </div> > </body> No, more like this: <img src="image.jpg" style="will-change: transform"> Comment on attachment 304176 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=304176&action=review > Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:385 > + // CGImageSourceCreateThumbnailAtIndex() returns a CGImage with the image native size > + // regardless of the subsamplingLevel unless kCGImageSourceSubsampleFactor is passed. > + // Here we are trying to see which size is smaller: the image native size or the > + // sizeForDrawing. If we want a CGImage with the image native size, sizeForDrawing will > + // not passed. So we need to get the image native size with the default subsampling and > + // then compare it with sizeForDrawing. Much clearer, thanks! (In reply to comment #102) > (In reply to comment #101) > > (In reply to comment #98) > > > Comment on attachment 304164 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=304164&action=review > > > > > > We need to test that the right thing happens for a directly composited > > > image, but otherwise this looks OK. > > > > > > > Did you mean something like this? > > > > <style> > > .transformed { > > transform: scale(2); > > transform-origin: top left; > > width: 400px; > > height: 400px; > > } > > </style> > > <body> > > <div class="transformed"> > > <img src="image.jpg"> > > </div> > > </body> > > > No, more like this: > > <img src="image.jpg" style="will-change: transform"> This scenario runs correctly but through the synchronous decoding path. RenderLayerBacking::updateImageContents() calls GraphicsLayerCA::setContentsToImage() which calls BitmapImage::nativeImageForCurrentFrame(). This later function forces synchronous when it calls frameImageAtIndex with a null sizeForDrawing. Comment on attachment 304176 [details] Patch Clearing flags on attachment: 304176 Committed r213764: <http://trac.webkit.org/changeset/213764> All reviewed patches have been landed. Closing bug. (In reply to WebKit Commit Bot from comment #105) > Comment on attachment 304176 [details] > Patch > > Clearing flags on attachment: 304176 > > Committed r213764: <http://trac.webkit.org/changeset/213764> This caused bug 170107. |