Created attachment 131750 [details] Patch

Ping?

Comment on attachment 131750 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131750&action=review > Source/WebCore/rendering/RenderBoxModelObject.cpp:1084 > + IntRect alignedRect = pixelSnappedIntRect(paintRect); It should be named alignedPaintRect. Maybe even better: snappedPaintRect? > Source/WebCore/rendering/RenderBoxModelObject.cpp:1127 > + positioningAreaSize = IntSize(alignedRect.width() - left - right, alignedRect.height() - top - bottom); Is it possible that we would want to snap after doing the subtractions as all the directions are LayoutUnits? Or is it better to be coherent with the width / height snapping above? > Source/WebCore/rendering/RenderBoxModelObject.cpp:1142 > + geometry.setPhaseX(geometry.tileSize().width() ? layoutMod(geometry.tileSize().width() - (xPosition + left), geometry.tileSize().width()) : zeroLayoutUnit); BackgroundImageGeometry::setPhaseX takes an |int| not a LayoutUnit. AFAICT we want to keep the integer there as we are manipulating a replaced element. Is this really what we want to do here? > Source/WebCore/rendering/RenderBoxModelObject.cpp:1148 > + geometry.setPhaseY(geometry.tileSize().height() ? layoutMod(geometry.tileSize().height() - (yPosition + top), geometry.tileSize().height()) : zeroLayoutUnit); Ditto.

Created attachment 132126 [details] Patch

Thanks for your thorough review Julien! (In reply to comment #3) > (From update of attachment 131750 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131750&action=review > > > Source/WebCore/rendering/RenderBoxModelObject.cpp:1084 > > + IntRect alignedRect = pixelSnappedIntRect(paintRect); > > It should be named alignedPaintRect. Maybe even better: snappedPaintRect? Renamed to snappedPaintRect > > > Source/WebCore/rendering/RenderBoxModelObject.cpp:1127 > > + positioningAreaSize = IntSize(alignedRect.width() - left - right, alignedRect.height() - top - bottom); > > Is it possible that we would want to snap after doing the subtractions as all the directions are LayoutUnits? Or is it better to be coherent with the width / height snapping above? Good idea, the padding values could have subpixel precision and would be floored otherwise. Changed to snap after subtracting the padding/border. > > > Source/WebCore/rendering/RenderBoxModelObject.cpp:1142 > > + geometry.setPhaseX(geometry.tileSize().width() ? layoutMod(geometry.tileSize().width() - (xPosition + left), geometry.tileSize().width()) : zeroLayoutUnit); > > BackgroundImageGeometry::setPhaseX takes an |int| not a LayoutUnit. AFAICT we want to keep the integer there as we are manipulating a replaced element. Is this really what we want to do here? Changed this to use integers throughout.

Comment on attachment 132126 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132126&action=review > Source/WebCore/rendering/RenderBoxModelObject.cpp:1144 > - geometry.setPhaseX(geometry.tileSize().width() ? layoutMod(geometry.tileSize().width() - (xPosition + left), geometry.tileSize().width()) : LayoutUnit(0)); > + geometry.setPhaseX(geometry.tileSize().width() ? geometry.tileSize().width() - (xPosition + left) % geometry.tileSize().width() : 0); Those 2 lines are not equivalent due to % being of higher priority that -. It's sad that our tests are not catching that. Extra points if you can find a test to catch that regression. > Source/WebCore/rendering/RenderBoxModelObject.cpp:1150 > + geometry.setPhaseY(geometry.tileSize().height() ? geometry.tileSize().height() - (yPosition + top) % geometry.tileSize().height() : 0); Ditto.

(In reply to comment #6) > (From update of attachment 132126 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=132126&action=review > > > Source/WebCore/rendering/RenderBoxModelObject.cpp:1144 > > - geometry.setPhaseX(geometry.tileSize().width() ? layoutMod(geometry.tileSize().width() - (xPosition + left), geometry.tileSize().width()) : LayoutUnit(0)); > > + geometry.setPhaseX(geometry.tileSize().width() ? geometry.tileSize().width() - (xPosition + left) % geometry.tileSize().width() : 0); > > Those 2 lines are not equivalent due to % being of higher priority that -. It's sad that our tests are not catching that. Extra points if you can find a test to catch that regression. > > > Source/WebCore/rendering/RenderBoxModelObject.cpp:1150 > > + geometry.setPhaseY(geometry.tileSize().height() ? geometry.tileSize().height() - (yPosition + top) % geometry.tileSize().height() : 0); > > Ditto. Good catch but this actually restores the code to the exact same state as it was before we changed it to use LayoutUnits. Which means that this been broken for the last seven months (!). http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderBoxModelObject.cpp?rev=92529#L932

Comment on attachment 132126 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132126&action=review >>> Source/WebCore/rendering/RenderBoxModelObject.cpp:1144 >>> + geometry.setPhaseX(geometry.tileSize().width() ? geometry.tileSize().width() - (xPosition + left) % geometry.tileSize().width() : 0); >> >> Those 2 lines are not equivalent due to % being of higher priority that -. It's sad that our tests are not catching that. Extra points if you can find a test to catch that regression. > > Good catch but this actually restores the code to the exact same state as it was before we changed it to use LayoutUnits. Which means that this been broken for the last seven months (!). > > http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderBoxModelObject.cpp?rev=92529#L932 This seems difficult to believe...!

Having looked into this in detail it turns out to be quite interesting, the way this is computed means that the two calculations will produce different numbers in cases where the background offset is larger than the size of the image. This isn't very common in the wild but is certainly valid. However it turns out that this doesn't effect the visual representation at all as it will be off by the full width (or height) of the image, resulting in the same offset and thus the same rendering.

Comment on attachment 132126 [details] Patch > However it turns out that this doesn't effect the visual representation at all as it will be off by the full width (or height) of the image, resulting in the same offset and thus the same rendering. OK... Could you file about a bug about this craziness / the fact that calculateBackgroundImageGeometry code should definitely be made less error-prone?

Filed bug 81396 to track the calculateBackgroundImageGeometry crazies.

Comment on attachment 132126 [details] Patch Clearing flags on attachment: 132126 Committed r111066: <http://trac.webkit.org/changeset/111066>

All reviewed patches have been landed. Closing bug.