Bug 27570

Also the round keyword. Currently they are not even dropped, bu treated like no-repeat which is really lame.

Created attachment 99695 [details] Patch

The patch should fix the following tests: http://samples.msdn.microsoft.com/ietestcenter/css3/bordersbackgrounds/background-repeat-space-padding-box.htm http://samples.msdn.microsoft.com/ietestcenter/css3/bordersbackgrounds/background_repeat_space_border_box.htm http://samples.msdn.microsoft.com/ietestcenter/css3/bordersbackgrounds/background_repeat_space_content_box.htm

Attachment 99695 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/rendering/RenderBoxModelObject.cpp:850: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 99696 [details] Fixed style

Comment on attachment 99696 [details] Fixed style Attachment 99696 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8985356

Created attachment 99697 [details] Patch

Comment on attachment 99697 [details] Patch Attachment 99697 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/8982738

Created attachment 99701 [details] Patch

Comment on attachment 99701 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99701&action=review Hi Alexandru, Thank you doing this. I try to give some feedback. > Source/WebCore/rendering/RenderBoxModelObject.cpp:768 > LayoutSize tileSize; How about start with bringing these parameters together into a class, namely BackgroundImageGeometry, in a separate patch? It would make this change clearer and smaller. > Source/WebCore/rendering/RenderBoxModelObject.h:137 > + bool calculateBackgroundImageGeometry(const FillLayer*, const IntRect& paintRect, IntRect& destRect, IntPoint& phase, IntSize& tileSize, IntSize& padding); How about to define overloaded version which doesn't have |padding| parameter. It looks many caller site just passes dummy value, which makes code complicated and makes this change larger than necessary.

(In reply to comment #10) > How about start with bringing these parameters together into a class, namely BackgroundImageGeometry, in a separate patch? I will post a patch with that. > How about to define overloaded version which doesn't have |padding| parameter. If I do the first comment, this wouldn't be needed anymore. Do you know if Skia can render spaces between pattern tiles? I only implemented it for CoreGraphics. The workaround would be to render the pattern in WebKit.

I've added https://bugs.webkit.org/show_bug.cgi?id=63987 and posted a patch to move the arguments into a class.

(In reply to comment #11) > (In reply to comment #10) > > > How about start with bringing these parameters together into a class, namely BackgroundImageGeometry, in a separate patch? > > I will post a patch with that. Thanks! I did have some feedback on Bug 63987.

> Do you know if Skia can render spaces between pattern tiles? I only implemented it for CoreGraphics. The workaround would be to render the pattern in WebKit. I had a quick glance on skia and couldn't find tiling with margins. If other non-CG backends also don't have the functionality, having it on WebCore would be helpful for these backends.

Created attachment 101433 [details] Patch

Created attachment 101435 [details] Patch Rebased the patch

The builds also failed on trunk. The fail is unrelated to this patch. http://build.webkit.org/builders/Leopard%20Intel%20Release%20%28Build%29/builds/37086/steps/compile-webkit/logs/stdio JavaScriptCore/runtime/ObjectPrototype.cpp: In function 'JSC::EncodedJSValue JSC::objectProtoFuncToString(JSC::ExecState*)': JavaScriptCore/runtime/ObjectPrototype.cpp:198: error: invalid conversion from 'JSC::JSString*' to 'JSC::EncodedJSValue'

Created attachment 101572 [details] Patch Rebased to latest tree.

Created attachment 101573 [details] Patch Rebased again.

Comment on attachment 101573 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101573&action=review > Source/WebCore/platform/graphics/GraphicsContext.h:315 > + const IntSize& tilePadding, CompositeOperator, bool useLowQualityScale); "useLowQualityScale" is unfortunate. It would be great if you pass InterpolationQuality directly. That kind of change should have separate bug though. > Source/WebCore/platform/graphics/Image.cpp:108 > +{ Is it possible to extend drawTiled() to support spacing instead of defining another yet similar method? How about to add a portable path like this on drawTitled(), for call with non-zero padding? > Source/WebCore/platform/graphics/Image.cpp:123 > + scaledTileSize.height() / intrinsicTileSize.height()); Is this division safe? > Source/WebCore/rendering/RenderBoxModelObject.cpp:875 > +bool RenderBoxModelObject::calculateFillTilePadding(EFillRepeat repeatX, EFillRepeat repeatY, const IntSize& containingBox, BackgroundImageGeometry& geometry) It looks this method is part of BackgroundImageGeometry? > Source/WebCore/rendering/RenderBoxModelObject.cpp:985 > + switch (backgroundRepeatX) { Can this switch statement be a method of BackgroundImageGeometry? > Source/WebCore/rendering/RenderBoxModelObject.cpp:998 > + switch (backgroundRepeatY) { Ditto.

Created attachment 101976 [details] Patch

(In reply to comment #20) > "useLowQualityScale" is unfortunate. It would be great if you pass InterpolationQuality directly. > That kind of change should have separate bug though. I will add a bug for that. > Is it possible to extend drawTiled() to support spacing instead of defining another yet similar method? > How about to add a portable path like this on drawTitled(), for call with non-zero padding? Did that in the last patch. > Is this division safe? The latest patch doesn't touch that area anymore. It should be safe because the image should have at least 1px by 1px size. > It looks this method is part of BackgroundImageGeometry? Yes, it also makes it nicer. Moved it to BackgroundImageGeometry. > Can this switch statement be a method of BackgroundImageGeometry? Yes, moved that too. Also removed unused methods from BackgroundImageGeometry, because I can access the fields directly now.

CCing smfr@ as an expert on this area, I missed to CC him...

Comment on attachment 101976 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101976&action=review The code looks almost fine to me. Could you add some more tests to cover edge cases like: - for RoundFill, the case for - Different repeat-x repeat-y combinations - "background-position", which should be ignored (according to the spec.) - A background area which is smaller than the background image - A background area which is as small as that the image cannot repeat twice. - background-origin: border-box Some of them looks OK to get them together. > LayoutTests/fast/css/background-repeat-round.html:10 > + } Could you make it clear that the image scaling happens? > Source/WebCore/platform/graphics/Image.cpp:151 > + ceilf()? > Source/WebCore/platform/graphics/Image.cpp:158 > + Could you use FloatRect constructor to set quad values instead of setting it individually? > Source/WebCore/platform/graphics/Image.cpp:174 > + visibleSrcRect.setHeight(clippedRect.height() / scale.height()); Use FloatRect constructor? > Source/WebCore/rendering/RenderBoxModelObject.cpp:1005 > + return; Is it OK to return here?

Comment on attachment 101976 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101976&action=review >> LayoutTests/fast/css/background-repeat-round.html:10 >> + } > > Could you make it clear that the image scaling happens? Please put spaces after the colons in this and the other tests.

Created attachment 102009 [details] Patch Added new tests. Included fixes for feedback.

Comment on attachment 102009 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102009&action=review > Source/WebCore/platform/graphics/Image.cpp:183 > + for (int x = 0; x <= repeatX; ++x) { > + for (int y = 0; y <= repeatY; ++y) { > + FloatRect tileRect; > + tileRect.setX(destRect.x() + fmodf(fmodf(-srcPoint.x(), tileRealSize.width()) - tileRealSize.width(), tileRealSize.width())); > + tileRect.setY(destRect.y() + fmodf(fmodf(-srcPoint.y(), tileRealSize.height()) - tileRealSize.height(), tileRealSize.height())); > + tileRect.setSize(scaledTileSize); > + > + tileRect.move(tileRealSize.width() * x, tileRealSize.height() * y); > + > + FloatRect clippedRect = tileRect; > + clippedRect.intersect(destRect); > + > + if (clippedRect.isEmpty()) > + continue; > + > + if (usesSolidColor) > + fillWithSolidColor(ctxt, clippedRect, solidColor(), styleColorSpace, op); > + else { > + FloatRect visibleSrcRect((clippedRect.x() - tileRect.x()) / scale.width(), > + (clippedRect.y() - tileRect.y()) / scale.height(), > + clippedRect.width() / scale.width(), > + clippedRect.height() / scale.height()); > + draw(ctxt, clippedRect, visibleSrcRect, styleColorSpace, op); > + } > + } > + } I'm concerned about the performance implications of this simple loop. Seems like you could fall into this slow tiling when you have a small background image with tiling over a large div (say a 1x1 px image with a 1px space over a 1000x1000 div).

Comment on attachment 102009 [details] Patch Attachment 102009 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9253346 New failing tests: fast/css/background-repeat-round-space.html fast/css/background-repeat-round-size.html fast/canvas/webgl/get-active-test.html fast/css/background-repeat-round-no-repeat.html fast/css/background-repeat-round-repeat.html fast/css/background-repeat-round.html fast/css/background-repeat-space.html

(In reply to comment #27) > (From update of attachment 102009 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=102009&action=review > I'm concerned about the performance implications of this simple loop. Seems like you could fall into this slow tiling when you have a small background image with tiling over a large div (say a 1x1 px image with a 1px space over a 1000x1000 div). That slow path is there just to help graphics libraries that do not have native support to render patterns with padding (eg. Skia, Cairo). CoreGraphics can draw patterns with different spacing, so I will add the fast path in a different patch/bug. Another solution might be to compute an image in memory with the size of the tile+padding and draw that using the native library.

> That slow path is there just to help graphics libraries that do not have native support to render patterns with padding (eg. Skia, Cairo). CoreGraphics can draw patterns with different spacing, so I will add the fast path in a different patch/bug. > So could you - file a bug for make CoreGraphics the fast path - add FIXME comment there with a link to the filed bug? I will make it easy to track the intention.

And please mark new tests as failed on non-mac ports.

Note that border-image now has a space value that needs to be implemented as well. I don't know if this bug can cover that work also or if we should just file a new one.

<rdar://problem/10218262>

Comment on attachment 102009 [details] Patch Based on Simon's comment and the EWS failing, I think this will need another pass. I haven't looked closely at the code though.

Just wanted to update that Safari is still facing lot of test in WPT: https://wpt.fyi/results/css/css-backgrounds?label=master&label=experimental&aligned&q=background-repeat-space I think this patch has not been landed or it has not been fixed. Just wanted to share an update. Thanks!