Bug 119324

Created attachment 208098 [details] Patch

Comment on attachment 208098 [details] Patch Attachment 208098 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1334921 New failing tests: fast/gradients/gradient-on-pseudoelement-crash.html

Created attachment 208102 [details] Archive of layout-test-results from webkit-ews-08 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.4

I don't think the failing test has anything to do with my patch, it also fails when running it on a branch which doesn't have the repeat space changes.

(In reply to comment #4) > I don't think the failing test has anything to do with my patch, it also fails when running it on a branch which doesn't have the repeat space changes. The background code can be fragile. Especially gradients code is likely to be caused by changes to background. Can you elaborate and make sure that it is not your fault?

Comment on attachment 208098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208098&action=review r- because of some change request. I didn't check the change in ModelObject yet. > Source/WebCore/ChangeLog:6 > + Reviewed by NOBODY (OOPS!). You need to explain what you do, why you do it and why this is the right thing to archive the right behavior. > Source/WebCore/ChangeLog:24 > + (WebCore::RenderBoxModelObject::paintFillLayerExtended): > + (WebCore::RenderBoxModelObject::calculateBackgroundImageGeometry): Smaller inline comments can help to understand what you do. > Source/WebCore/platform/graphics/cg/ImageCG.cpp:136 > + // On Leopard and newer, this code now only runs for partially decoded images whose buffers do not yet match the overall size of the image. > + static const CGPatternCallbacks patternCallbacks = { 0, drawPatternCallback, 0 }; > + CGAffineTransform matrix = CGAffineTransformMake(narrowPrecisionToCGFloat(patternTransform.a()), 0, 0, narrowPrecisionToCGFloat(patternTransform.d()), adjustedX, adjustedY); > + matrix = CGAffineTransformConcat(matrix, CGContextGetCTM(context)); > + // The top of a partially-decoded image is drawn at the bottom of the tile. Map it to the top. > + matrix = CGAffineTransformTranslate(matrix, 0, size().height() - h); > + RetainPtr<CGPatternRef> pattern = adoptCF(CGPatternCreate(subImage.get(), CGRectMake(0, 0, tileRect.width(), tileRect.height()), > + matrix, tileRect.width() + spaceSize().width(), tileRect.height() + spaceSize().height(), > + kCGPatternTilingConstantSpacing, true, &patternCallbacks)); > + if (!pattern) > + return; > + It would be great if you can use the Pattern class already in WebKit to do that, so that it works in all ports, not just Mac. You may need to add some more code to Pattern to make that possible. Can you check this first please? Resetting review flag for now. > LayoutTests/css3/background/background-repeat-border-expected.html:16 > + var urls = Array(), size = Array(), position = Array(), repeat = Array(), > + origin = Array(), clip = Array(); Does that all need to be JS? Can't you just write a simple test case to make sure that it works? > LayoutTests/css3/background/background-repeat-border-expected.html:18 > + function addMasks() { If you are testing background, then mask might be misleading.

> > Source/WebCore/platform/graphics/cg/ImageCG.cpp:136 > > + // On Leopard and newer, this code now only runs for partially decoded images whose buffers do not yet match the overall size of the image. > > + static const CGPatternCallbacks patternCallbacks = { 0, drawPatternCallback, 0 }; > > + CGAffineTransform matrix = CGAffineTransformMake(narrowPrecisionToCGFloat(patternTransform.a()), 0, 0, narrowPrecisionToCGFloat(patternTransform.d()), adjustedX, adjustedY); > > + matrix = CGAffineTransformConcat(matrix, CGContextGetCTM(context)); > > + // The top of a partially-decoded image is drawn at the bottom of the tile. Map it to the top. > > + matrix = CGAffineTransformTranslate(matrix, 0, size().height() - h); > > + RetainPtr<CGPatternRef> pattern = adoptCF(CGPatternCreate(subImage.get(), CGRectMake(0, 0, tileRect.width(), tileRect.height()), > > + matrix, tileRect.width() + spaceSize().width(), tileRect.height() + spaceSize().height(), > > + kCGPatternTilingConstantSpacing, true, &patternCallbacks)); > > + if (!pattern) > > + return; > > + > > It would be great if you can use the Pattern class already in WebKit to do that, so that it works in all ports, not just Mac. You may need to add some more code to Pattern to make that possible. Can you check this first please? Resetting review flag for now. That code was already there (except the adding of 'spaceSize'). I will try using the Pattern class but it would require some changes (you have to pass a CGImageRef to it which you currently cannot). > > LayoutTests/css3/background/background-repeat-border-expected.html:16 > > + var urls = Array(), size = Array(), position = Array(), repeat = Array(), > > + origin = Array(), clip = Array(); > > Does that all need to be JS? Can't you just write a simple test case to make sure that it works? I wanted some tests with masks which are being repeated a bigger number of times - adding 12 masks manually seemed ugly :). Though I can get rid of the repeat, origin and clip arrays, apparently I only need to specifiy them once.

Created attachment 208122 [details] Patch

Tried to change ImageCG::drawPattern to use the Pattern class, although I don't know how useful it will be - didn't figure out a better way to do it.

Created attachment 208396 [details] Patch

(In reply to comment #10) > Created an attachment (id=208396) [details] > Patch Added some more comments to the change logs.

Comment on attachment 208396 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208396&action=review Great patch! I think we run into a misunderstanding on the use of Pattern. It would be great if this could be done outside of the platform dependent code, so that it works on all platforms. > Source/WebCore/platform/graphics/Pattern.h:63 > + static PassRefPtr<Pattern> create(); Hm, you create a Pattern without initial data? Don't you alraedy have access to the Image at that point? > Source/WebCore/platform/graphics/cg/ImageCG.cpp:126 > + matrix = CGAffineTransformTranslate(matrix, 0, size().height() - h); > + RefPtr<Pattern> patternPtr = Pattern::create(); > + RetainPtr<CGPatternRef> pattern = adoptCF(patternPtr->createPlatformPattern(matrix, subImage.get(), tileRect, I thought more of using Pattern in the platform independent path. Here is does not make a lot of sense to wrap the Pattern class around the PlatformPattern. This can not be used by other ports either. I thought more in rendering code, where we initially call GraphicsContext background drawing routines. > Source/WebCore/platform/graphics/cg/ImageCG.cpp:143 > + CGRect r = CGContextGetClipBoundingBox(context); > + CGContextFillRect(context, r); Did you initialize the r for testing? You don't need to assign a new variable here. > Source/WebCore/platform/graphics/cg/PatternCG.cpp:90 > +CGPatternRef Pattern::createPlatformPattern(const AffineTransform& matrix, PlatformImagePtr patternImage, FloatRect tileRect, float xStep, float yStep) const You may want to pass a FloatSize instead of xStep and yStep. > Source/WebCore/rendering/RenderBoxModelObject.cpp:1323 > + geometry.setSpaceSize(FloatSize(0, 0)); just use FloatSize() > Source/WebCore/rendering/RenderBoxModelObject.cpp:1367 > + } else if (backgroundRepeatY == SpaceFill) { > + if (fillTileSize.height() > 0) { can you combine that a one condition in the else if ?

Comment on attachment 208396 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208396&action=review We had longer discussions how the Pattern class can be refactored to represent the spacing. We realize that there are some tradeoffs to make like storing more member variables in the frequently used Pattern class. On the other hand there are no benefits for the other ports that still need to modify the image buffers. CG provides spacing in the internal pattern logic, the other ports don't. Because of that, I think it doesn't make sense to modify Pattern after all and just do the necessary changes in ImageCG. Some nips: > Source/WebCore/ChangeLog:11 > + [CSS Masking] -webkit-mask-repeat: space does not work > + Added the space option to repeat property using the createPlatformPattern function. > + The space mask-repeat repeats the mask as often as will fit within the background positioning > + area without being clipped and then spaces the masks so that they fill the area. > + Changed the Pattern class so that createPlatformPattern can be called from the ImageCG use-case. > + https://bugs.webkit.org/show_bug.cgi?id=119324 > + > + Reviewed by NOBODY (OOPS!). > + The format of the changelog is still a bit off. s/repeat property/background-repeat and -webkit-mask-repeat properties/ The space mask-repeat... With the property value 'space', the background or mask image gets repeated as often as it fits within the background positioning area. The repeated images are spaced equally to fill the unused area. > Source/WebCore/ChangeLog:19 > + * WebCore.order: added the symbol for the new Pattern::create method Sentences please. > Source/WebCore/ChangeLog:22 > + * platform/graphics/Image.h: add space property to Image class Ditto. > Source/WebCore/ChangeLog:25 > + * platform/graphics/Pattern.cpp: added a new createPlatformPattern method to be called from ImageCG Ditto. > Source/WebCore/ChangeLog:29 > + * platform/graphics/cg/ImageCG.cpp: added the space values to createPlatformPattern Ditto. > Source/WebCore/ChangeLog:36 > + (WebCore::RenderBoxModelObject::paintFillLayerExtended): computed the space values on x and y axis > + (WebCore::RenderBoxModelObject::calculateBackgroundImageGeometry): passed the space values to the Image class > + * rendering/RenderBoxModelObject.h: add space property to BackgroundImageGeometry class Ditto. s/computed/Compute/ s/passed/Pass/ > Source/WebCore/rendering/RenderBoxModelObject.cpp:1344 > + int nrTiles = positioningAreaSize.width() / fillTileSize.width(); Please rename nrTiles. Not sure, maybe to numberOfTiles. > Source/WebCore/rendering/RenderBoxModelObject.cpp:1368 > + int nrTiles = positioningAreaSize.height() / fillTileSize.height(); Ditto. > Source/WebCore/rendering/RenderBoxModelObject.cpp:1377 > + int actualHeight = geometry.tileSize().height(); > + if (nrTiles > 1) { > + int space = roundToInt((float)(positioningAreaSize.height() - nrTiles * fillTileSize.height()) / (nrTiles - 1)); > + geometry.setSpaceSize(FloatSize(geometry.spaceSize().width(), space)); > + > + actualHeight += space; > + } else > + geometry.setSpaceSize(FloatSize(geometry.spaceSize().width(), 0)); That may should go into an inline function and reused by both: width and height.

Created attachment 209964 [details] Patch

(In reply to comment #14) > Created an attachment (id=209964) [details] > Patch Also passed the space values to ImageBuffer and SVGImage classes so that it can work with gradients and svg images - also changed two tests to use these two as masks.

Comment on attachment 209964 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209964&action=review r=me > Source/WebCore/platform/graphics/ImageBuffer.h:135 > + { > + m_space = space; > + } one liners are usually done without wrapping.

Comment on attachment 209964 [details] Patch Clearing flags on attachment: 209964 Committed r154875: <http://trac.webkit.org/changeset/154875>

All reviewed patches have been landed. Closing bug.