Created attachment 248890 [details] test case Open the attached test case. Expected: No red color is displayed and the size of the green rectangle should be 300x100 pixels. Switching from SVG with intrinsic size to an SVG with no intrinsic size or vice versa seems to work as expected. The problem happens when switching between two images with no-intrisinc sizes back and forth. In this case we use the old layout of the SVG image but we set the SVG size to the default SVG intrinsic size which is 300x150 pixels. In the attached test case, the container size is 300x100 pixels. The old layout can display the SVG in 300x100 pixels. But because we change the image size to be 300x150 pixels, the SVG gets scaled down by (1,100/150).
Another example of this problem can be seen in http://snapsvg.io/demos/. When switching between the items in the side bar on the left. Currently we do not display the SVG with the smaller size and we leave dirt behind from the SVG with the bigger size. This bug addresses the layout issue which is responsible for not showing the smaller SVG image.
Created attachment 248947 [details] Patch
Created attachment 248954 [details] Patch
Comment on attachment 248954 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248954&action=review > Source/WebCore/rendering/RenderImage.cpp:350 > + LayoutRect repaintRect; > + if (rect) { > + // The image changed rect is in source image coordinates (pre-zooming), > + // so map from the bounds of the image to the contentsBox. > + repaintRect = enclosingIntRect(mapRect(*rect, FloatRect(FloatPoint(), imageResource().imageSize(1.0f)), contentBoxRect())); > + // Guard against too-large changed rects. > + repaintRect.intersect(contentBoxRect()); > + } else > + repaintRect = contentBoxRect(); > > - repaintRectangle(repaintRect); > + repaintRectangle(repaintRect); I would have written this more like this: LayoutRect repaintRect = contentBoxRect(); if (rect) { // The image changed rect is in source image coordinates (pre-zooming), // so map from the bounds of the image to the contentsBox. repaintRect.intersect(enclosingIntRect(mapRect(*rect, FloatRect(FloatPoint(), imageResource().imageSize(1)), repaintRect))); } But also, I don’t understand why there is a call to enclosingIntRect here. > Source/WebCore/rendering/RenderImage.h:51 > + ImageSizeChangeType setImageSizeForAltText(CachedImage* newImage = 0); nullptr > Source/WebCore/rendering/RenderImage.h:115 > + void repaintOrMarkForLayout(ImageSizeChangeType, const IntRect* = 0); nullptr
Comment on attachment 248954 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248954&action=review >> Source/WebCore/rendering/RenderImage.cpp:350 >> + repaintRectangle(repaintRect); > > I would have written this more like this: > > LayoutRect repaintRect = contentBoxRect(); > if (rect) { > // The image changed rect is in source image coordinates (pre-zooming), > // so map from the bounds of the image to the contentsBox. > repaintRect.intersect(enclosingIntRect(mapRect(*rect, FloatRect(FloatPoint(), imageResource().imageSize(1)), repaintRect))); > } > > But also, I don’t understand why there is a call to enclosingIntRect here. Done. I think enclosingIntRect is used because LayoutRect::interest() takes a LayoutRect& as a parameter. And mapRect takes FloatRect& and returns also FloatRect. There is no implicit copy constructor in LayoutRect to convert from FloatRect to LayoutRect. But there is copy constructor to convert from IntRect to LayoutRect. When I remove the call to enclosingIntRect(), the compiler gives the error: "No viable conversion from FloatRect to const LayoutRect" >> Source/WebCore/rendering/RenderImage.h:51 >> + ImageSizeChangeType setImageSizeForAltText(CachedImage* newImage = 0); > > nullptr Done. >> Source/WebCore/rendering/RenderImage.h:115 >> + void repaintOrMarkForLayout(ImageSizeChangeType, const IntRect* = 0); > > nullptr Done.
Created attachment 248989 [details] Patch
Created attachment 248992 [details] Patch
(In reply to comment #5) > Comment on attachment 248954 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=248954&action=review > > >> Source/WebCore/rendering/RenderImage.cpp:350 > >> + repaintRectangle(repaintRect); > > > > I would have written this more like this: > > > > LayoutRect repaintRect = contentBoxRect(); > > if (rect) { > > // The image changed rect is in source image coordinates (pre-zooming), > > // so map from the bounds of the image to the contentsBox. > > repaintRect.intersect(enclosingIntRect(mapRect(*rect, FloatRect(FloatPoint(), imageResource().imageSize(1)), repaintRect))); > > } > > > > But also, I don’t understand why there is a call to enclosingIntRect here. > > Done. > > I think enclosingIntRect is used because LayoutRect::interest() takes a > LayoutRect& as a parameter. And mapRect takes FloatRect& and returns also > FloatRect. There is no implicit copy constructor in LayoutRect to convert > from FloatRect to LayoutRect. That's because this is lossy. LayoutRect has a resolution of 1/64th pixels. You can explicitly create a LayoutRect from a FloatRect though. > But there is copy constructor to convert from > IntRect to LayoutRect. Which is non-lossy. > When I remove the call to enclosingIntRect(), the > compiler gives the error: "No viable conversion from FloatRect to const LayoutRect"
Comment on attachment 248992 [details] Patch Rejecting attachment 248992 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 248992, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Full output: http://webkit-queues.appspot.com/results/4796037164171264
Created attachment 249005 [details] Patch
Comment on attachment 249005 [details] Patch Clearing flags on attachment: 249005 Committed r181720: <http://trac.webkit.org/changeset/181720>
All reviewed patches have been landed. Closing bug.
This broke GTK+ build. It's been fixed in: http://trac.webkit.org/changeset/181729
(In reply to comment #13) > This broke GTK+ build. It's been fixed in: > http://trac.webkit.org/changeset/181729 bug142859 fixed it too 3 hours before, but there were nobody to review it. :(
(In reply to comment #14) > (In reply to comment #13) > > This broke GTK+ build. It's been fixed in: > > http://trac.webkit.org/changeset/181729 > > bug142859 fixed it too 3 hours before, but there were nobody to review it. :( Sorry, didn't realize there was a fix already. It was so simple that I just go ahead as the GTK+ EWS was getting into troubles.
(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #13) > > > This broke GTK+ build. It's been fixed in: > > > http://trac.webkit.org/changeset/181729 > > > > bug142859 fixed it too 3 hours before, but there were nobody to review it. :( > > Sorry, didn't realize there was a fix already. > It was so simple that I just go ahead as the GTK+ EWS was getting into > troubles. Not problem, thanks for the fix. :) I commented ":(" because the fix was already done, but nobody reviewed and landed in for a long period. To avoid parallel fixing and long build break period I suggested commenting the original bug and pinging reviewers on #webkit until it is reviewed and landed.