void drawImage(Image*, ColorSpace styleColorSpace, const FloatPoint&, CompositeOperator = CompositeSourceOver, ImageOrientationDescription = ImageOrientationDescription()); void drawImage(Image*, ColorSpace styleColorSpace, const FloatRect&, CompositeOperator = CompositeSourceOver, ImageOrientationDescription = ImageOrientationDescription(), bool useLowQualityScale = false); void drawImage(Image*, ColorSpace styleColorSpace, const FloatPoint& destPoint, const FloatRect& srcRect, CompositeOperator = CompositeSourceOver, ImageOrientationDescription = ImageOrientationDescription()); void drawImage(Image*, ColorSpace styleColorSpace, const FloatRect& destRect, const FloatRect& srcRect, CompositeOperator = CompositeSourceOver, BlendMode = BlendModeNormal, ImageOrientationDescription = ImageOrientationDescription(), bool useLowQualityScale = false); void drawTiledImage(Image*, ColorSpace styleColorSpace, const FloatRect& destRect, const FloatPoint& srcPoint, const FloatSize& tileSize, CompositeOperator = CompositeSourceOver, bool useLowQualityScale = false, BlendMode = BlendModeNormal); void drawTiledImage(Image*, ColorSpace styleColorSpace, const FloatRect& destRect, const FloatRect& srcRect, const FloatSize& tileScaleFactor, Image::TileRule hRule = Image::StretchTile, Image::TileRule vRule = Image::StretchTile, CompositeOperator = CompositeSourceOver, bool useLowQualityScale = false); void drawImageBuffer(ImageBuffer*, ColorSpace styleColorSpace, const FloatPoint&, CompositeOperator = CompositeSourceOver, BlendMode = BlendModeNormal); void drawImageBuffer(ImageBuffer*, ColorSpace styleColorSpace, const FloatRect&, CompositeOperator = CompositeSourceOver, BlendMode = BlendModeNormal, bool useLowQualityScale = false); void drawImageBuffer(ImageBuffer*, ColorSpace styleColorSpace, const FloatPoint& destPoint, const FloatRect& srcRect, CompositeOperator = CompositeSourceOver, BlendMode = BlendModeNormal); void drawImageBuffer(ImageBuffer*, ColorSpace styleColorSpace, const FloatRect& destRect, const FloatRect& srcRect, CompositeOperator = CompositeSourceOver, BlendMode = BlendModeNormal, bool useLowQualityScale = false); The image related drawing property list keeps growing and results in multiple overloading functions with seemingly random default parameters. Some kind of encapsulation should help on this.
Created attachment 232346 [details] Patch
Attachment 232346 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/GraphicsContext.h:199: Wrong number of spaces before statement. (expected: 16) [whitespace/indent] [4] ERROR: Source/WebCore/platform/graphics/GraphicsContext.h:200: Wrong number of spaces before statement. (expected: 16) [whitespace/indent] [4] ERROR: Source/WebCore/platform/graphics/GraphicsContext.h:201: Wrong number of spaces before statement. (expected: 16) [whitespace/indent] [4] ERROR: Source/WebCore/platform/graphics/GraphicsContext.h:202: Wrong number of spaces before statement. (expected: 16) [whitespace/indent] [4] Total errors found: 4 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 232354 [details] Patch
Attachment 232354 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/GraphicsContext.h:199: Wrong number of spaces before statement. (expected: 16) [whitespace/indent] [4] ERROR: Source/WebCore/platform/graphics/GraphicsContext.h:200: Wrong number of spaces before statement. (expected: 16) [whitespace/indent] [4] ERROR: Source/WebCore/platform/graphics/GraphicsContext.h:201: Wrong number of spaces before statement. (expected: 16) [whitespace/indent] [4] ERROR: Source/WebCore/platform/graphics/GraphicsContext.h:202: Wrong number of spaces before statement. (expected: 16) [whitespace/indent] [4] Total errors found: 4 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 232354 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232354&action=review Seems OK to bundle the arguments like this. We could also make overloaded constructors to make this a little less wordy for cases where only one argument has an unusual value. Today we have that for compositing mode, but we could also have it for the orientation description. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1655 > - c->drawImageBuffer(buffer, ColorSpaceDeviceRGB, bufferRect.location(), state().m_globalComposite); > + c->drawImageBuffer(buffer, ColorSpaceDeviceRGB, bufferRect.location(), ImagePaintingContext(state().m_globalComposite)); No need for this change. It should compile as-is. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1661 > - context->drawImage(image, styleColorSpace, dest, src, op); > + context->drawImage(image, styleColorSpace, dest, src, ImagePaintingContext(op)); No need for this change. It should compile as-is. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1666 > - context->drawImageBuffer(imageBuffer, styleColorSpace, dest, src, op); > + context->drawImageBuffer(imageBuffer, styleColorSpace, dest, src, ImagePaintingContext(op)); No need for this change. It should compile as-is. > Source/WebCore/platform/graphics/GraphicsContext.h:198 > + ImagePaintingContext(CompositeOperator compositeOperator = CompositeSourceOver, > + BlendMode blendMode = BlendModeNormal, ImageOrientationDescription orientationDescription = ImageOrientationDescription(), bool useLowQualityScale = false) Not sure why you broke the line where you did. > Source/WebCore/platform/graphics/GraphicsContext.h:323 > + void drawImage(Image*, ColorSpace, const FloatRect& dest, const FloatRect& src, const ImagePaintingContext& = ImagePaintingContext()); I suggest "destination" and "source" rather than "dst" and "src". We should also have all these functions take Image& and ImageBuffer& rather than pointers. > Source/WebCore/platform/graphics/filters/FEComposite.cpp:307 > - filterContext->drawImageBuffer(imageBuffer2, ColorSpaceDeviceRGB, drawingRegionOfInputImage(in2->absolutePaintRect()), IntRect(IntPoint(), imageBuffer2->logicalSize()), CompositeDestinationOut); > + filterContext->drawImageBuffer(imageBuffer2, ColorSpaceDeviceRGB, drawingRegionOfInputImage(in2->absolutePaintRect()), IntRect(IntPoint(), imageBuffer2->logicalSize()), > + ImagePaintingContext(CompositeDestinationOut)); No need to change this. It should compile as is. > Source/WebCore/platform/graphics/filters/FEComposite.cpp:312 > - filterContext->drawImageBuffer(imageBuffer, ColorSpaceDeviceRGB, drawingRegionOfInputImage(in->absolutePaintRect()), IntRect(IntPoint(), imageBuffer->logicalSize()), CompositeSourceAtop); > + filterContext->drawImageBuffer(imageBuffer, ColorSpaceDeviceRGB, drawingRegionOfInputImage(in->absolutePaintRect()), IntRect(IntPoint(), imageBuffer->logicalSize()), > + ImagePaintingContext(CompositeSourceAtop)); No need to change this. It should compile as is. > Source/WebCore/platform/graphics/filters/FEComposite.cpp:317 > - filterContext->drawImageBuffer(imageBuffer, ColorSpaceDeviceRGB, drawingRegionOfInputImage(in->absolutePaintRect()), IntRect(IntPoint(), imageBuffer->logicalSize()), CompositeXOR); > + filterContext->drawImageBuffer(imageBuffer, ColorSpaceDeviceRGB, drawingRegionOfInputImage(in->absolutePaintRect()), IntRect(IntPoint(), imageBuffer->logicalSize()), > + ImagePaintingContext(CompositeXOR)); No need to change this. It should compile as is. > Source/WebCore/rendering/RenderBoxModelObject.cpp:1332 > - pixelSnappedForPainting(0, 0, leftSlice, topSlice, deviceScaleFactor), op); > + pixelSnappedForPainting(0, 0, leftSlice, topSlice, deviceScaleFactor), ImagePaintingContext(op)); No need to change this. It should compile as is. > Source/WebCore/rendering/RenderBoxModelObject.cpp:1338 > - pixelSnappedForPainting(0, imageHeight - bottomSlice, leftSlice, bottomSlice, deviceScaleFactor), op); > + pixelSnappedForPainting(0, imageHeight - bottomSlice, leftSlice, bottomSlice, deviceScaleFactor), ImagePaintingContext(op)); No need to change this. It should compile as is. > Source/WebCore/rendering/RenderBoxModelObject.cpp:1345 > - pixelSnappedForPainting(0, topSlice, leftSlice, sourceHeight, deviceScaleFactor), FloatSize(leftSideScale, leftSideScale), Image::StretchTile, (Image::TileRule)vRule, op); > + pixelSnappedForPainting(0, topSlice, leftSlice, sourceHeight, deviceScaleFactor), FloatSize(leftSideScale, leftSideScale), > + Image::StretchTile, (Image::TileRule)vRule, ImagePaintingContext(op)); No need to change this. It should compile as is. > Source/WebCore/rendering/RenderBoxModelObject.cpp:1354 > - pixelSnappedForPainting(imageWidth - rightSlice, 0, rightSlice, topSlice, deviceScaleFactor), op); > + pixelSnappedForPainting(imageWidth - rightSlice, 0, rightSlice, topSlice, deviceScaleFactor), ImagePaintingContext(op)); No need to change this. It should compile as is. > Source/WebCore/rendering/RenderBoxModelObject.cpp:1361 > - op); > + ImagePaintingContext(op)); No need to change this. It should compile as is. > Source/WebCore/rendering/RenderBoxModelObject.cpp:1366 > - graphicsContext->drawTiledImage(image.get(), colorSpace, pixelSnappedForPainting(borderImageRect.maxX() - rightWidth, y + topWidth, rightWidth, destinationHeight, deviceScaleFactor), > - pixelSnappedForPainting(imageWidth - rightSlice, topSlice, rightSlice, sourceHeight, deviceScaleFactor), FloatSize(rightSideScale, rightSideScale), > - Image::StretchTile, (Image::TileRule)vRule, op); > + graphicsContext->drawTiledImage(image.get(), colorSpace, pixelSnappedForPainting(borderImageRect.maxX() - rightWidth, y + topWidth, rightWidth, destinationHeight, deviceScaleFactor), pixelSnappedForPainting(imageWidth - rightSlice, topSlice, rightSlice, sourceHeight, deviceScaleFactor), > + FloatSize(rightSideScale, rightSideScale), Image::StretchTile, (Image::TileRule)vRule, ImagePaintingContext(op)); No need to change this. It should compile as is. > Source/WebCore/rendering/RenderBoxModelObject.cpp:1373 > graphicsContext->drawTiledImage(image.get(), colorSpace, pixelSnappedForPainting(x + leftWidth, y, destinationWidth, topWidth, deviceScaleFactor), > - pixelSnappedForPainting(leftSlice, 0, sourceWidth, topSlice, deviceScaleFactor), FloatSize(topSideScale, topSideScale), (Image::TileRule)hRule, Image::StretchTile, op); > + pixelSnappedForPainting(leftSlice, 0, sourceWidth, topSlice, deviceScaleFactor), FloatSize(topSideScale, topSideScale), > + (Image::TileRule)hRule, Image::StretchTile, ImagePaintingContext(op)); No need to change this. It should compile as is. > Source/WebCore/rendering/RenderBoxModelObject.cpp:1379 > - graphicsContext->drawTiledImage(image.get(), colorSpace, pixelSnappedForPainting(x + leftWidth, borderImageRect.maxY() - bottomWidth, destinationWidth, bottomWidth, deviceScaleFactor), > - pixelSnappedForPainting(leftSlice, imageHeight - bottomSlice, sourceWidth, bottomSlice, deviceScaleFactor), FloatSize(bottomSideScale, bottomSideScale), > - (Image::TileRule)hRule, Image::StretchTile, op); > + graphicsContext->drawTiledImage(image.get(), colorSpace, pixelSnappedForPainting(x + leftWidth, borderImageRect.maxY() - bottomWidth, destinationWidth, > + bottomWidth, deviceScaleFactor), pixelSnappedForPainting(leftSlice, imageHeight - bottomSlice, sourceWidth, bottomSlice, deviceScaleFactor), > + FloatSize(bottomSideScale, bottomSideScale), (Image::TileRule)hRule, Image::StretchTile, ImagePaintingContext(op)); No need to change this. It should compile as is. > Source/WebCore/rendering/RenderBoxModelObject.cpp:1406 > - middleScaleFactor, (Image::TileRule)hRule, (Image::TileRule)vRule, op); > + middleScaleFactor, (Image::TileRule)hRule, (Image::TileRule)vRule, ImagePaintingContext(op)); No need to change this. It should compile as is.
Created attachment 232362 [details] Patch
Comment on attachment 232362 [details] Patch EWS
(In reply to comment #5) > (From update of attachment 232354 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=232354&action=review > > Seems OK to bundle the arguments like this. We could also make overloaded constructors to make this a little less wordy for cases where only one argument has an unusual value. Today we have that for compositing mode, but we could also have it for the orientation description. Good idea. Done. > > > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1655 > > - c->drawImageBuffer(buffer, ColorSpaceDeviceRGB, bufferRect.location(), state().m_globalComposite); > > + c->drawImageBuffer(buffer, ColorSpaceDeviceRGB, bufferRect.location(), ImagePaintingContext(state().m_globalComposite)); > > No need for this change. It should compile as-is. Done. > > > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1661 > > - context->drawImage(image, styleColorSpace, dest, src, op); > > + context->drawImage(image, styleColorSpace, dest, src, ImagePaintingContext(op)); > > No need for this change. It should compile as-is. Done. > > > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1666 > > - context->drawImageBuffer(imageBuffer, styleColorSpace, dest, src, op); > > + context->drawImageBuffer(imageBuffer, styleColorSpace, dest, src, ImagePaintingContext(op)); > > No need for this change. It should compile as-is. Done. > > > Source/WebCore/platform/graphics/GraphicsContext.h:198 > > + ImagePaintingContext(CompositeOperator compositeOperator = CompositeSourceOver, > > + BlendMode blendMode = BlendModeNormal, ImageOrientationDescription orientationDescription = ImageOrientationDescription(), bool useLowQualityScale = false) > > Not sure why you broke the line where you did. Fixed. > > > Source/WebCore/platform/graphics/GraphicsContext.h:323 > > + void drawImage(Image*, ColorSpace, const FloatRect& dest, const FloatRect& src, const ImagePaintingContext& = ImagePaintingContext()); > > I suggest "destination" and "source" rather than "dst" and "src". Done. > > We should also have all these functions take Image& and ImageBuffer& rather than pointers. https://bugs.webkit.org/show_bug.cgi?id=133442 > > > Source/WebCore/platform/graphics/filters/FEComposite.cpp:307 > > - filterContext->drawImageBuffer(imageBuffer2, ColorSpaceDeviceRGB, drawingRegionOfInputImage(in2->absolutePaintRect()), IntRect(IntPoint(), imageBuffer2->logicalSize()), CompositeDestinationOut); > > + filterContext->drawImageBuffer(imageBuffer2, ColorSpaceDeviceRGB, drawingRegionOfInputImage(in2->absolutePaintRect()), IntRect(IntPoint(), imageBuffer2->logicalSize()), > > + ImagePaintingContext(CompositeDestinationOut)); > > No need to change this. It should compile as is. Done. > > > Source/WebCore/platform/graphics/filters/FEComposite.cpp:312 > > - filterContext->drawImageBuffer(imageBuffer, ColorSpaceDeviceRGB, drawingRegionOfInputImage(in->absolutePaintRect()), IntRect(IntPoint(), imageBuffer->logicalSize()), CompositeSourceAtop); > > + filterContext->drawImageBuffer(imageBuffer, ColorSpaceDeviceRGB, drawingRegionOfInputImage(in->absolutePaintRect()), IntRect(IntPoint(), imageBuffer->logicalSize()), > > + ImagePaintingContext(CompositeSourceAtop)); > > No need to change this. It should compile as is. Done. > > > Source/WebCore/platform/graphics/filters/FEComposite.cpp:317 > > - filterContext->drawImageBuffer(imageBuffer, ColorSpaceDeviceRGB, drawingRegionOfInputImage(in->absolutePaintRect()), IntRect(IntPoint(), imageBuffer->logicalSize()), CompositeXOR); > > + filterContext->drawImageBuffer(imageBuffer, ColorSpaceDeviceRGB, drawingRegionOfInputImage(in->absolutePaintRect()), IntRect(IntPoint(), imageBuffer->logicalSize()), > > + ImagePaintingContext(CompositeXOR)); > > No need to change this. It should compile as is. Done. > > > Source/WebCore/rendering/RenderBoxModelObject.cpp:1332 > > - pixelSnappedForPainting(0, 0, leftSlice, topSlice, deviceScaleFactor), op); > > + pixelSnappedForPainting(0, 0, leftSlice, topSlice, deviceScaleFactor), ImagePaintingContext(op)); > > No need to change this. It should compile as is. Done. > > > Source/WebCore/rendering/RenderBoxModelObject.cpp:1338 > > - pixelSnappedForPainting(0, imageHeight - bottomSlice, leftSlice, bottomSlice, deviceScaleFactor), op); > > + pixelSnappedForPainting(0, imageHeight - bottomSlice, leftSlice, bottomSlice, deviceScaleFactor), ImagePaintingContext(op)); > > No need to change this. It should compile as is. Done. > > > Source/WebCore/rendering/RenderBoxModelObject.cpp:1345 > > - pixelSnappedForPainting(0, topSlice, leftSlice, sourceHeight, deviceScaleFactor), FloatSize(leftSideScale, leftSideScale), Image::StretchTile, (Image::TileRule)vRule, op); > > + pixelSnappedForPainting(0, topSlice, leftSlice, sourceHeight, deviceScaleFactor), FloatSize(leftSideScale, leftSideScale), > > + Image::StretchTile, (Image::TileRule)vRule, ImagePaintingContext(op)); > > No need to change this. It should compile as is. Done. > > > Source/WebCore/rendering/RenderBoxModelObject.cpp:1354 > > - pixelSnappedForPainting(imageWidth - rightSlice, 0, rightSlice, topSlice, deviceScaleFactor), op); > > + pixelSnappedForPainting(imageWidth - rightSlice, 0, rightSlice, topSlice, deviceScaleFactor), ImagePaintingContext(op)); > > No need to change this. It should compile as is. Done. > > > Source/WebCore/rendering/RenderBoxModelObject.cpp:1361 > > - op); > > + ImagePaintingContext(op)); > > No need to change this. It should compile as is. Done. > > > Source/WebCore/rendering/RenderBoxModelObject.cpp:1366 > > - graphicsContext->drawTiledImage(image.get(), colorSpace, pixelSnappedForPainting(borderImageRect.maxX() - rightWidth, y + topWidth, rightWidth, destinationHeight, deviceScaleFactor), > > - pixelSnappedForPainting(imageWidth - rightSlice, topSlice, rightSlice, sourceHeight, deviceScaleFactor), FloatSize(rightSideScale, rightSideScale), > > - Image::StretchTile, (Image::TileRule)vRule, op); > > + graphicsContext->drawTiledImage(image.get(), colorSpace, pixelSnappedForPainting(borderImageRect.maxX() - rightWidth, y + topWidth, rightWidth, destinationHeight, deviceScaleFactor), pixelSnappedForPainting(imageWidth - rightSlice, topSlice, rightSlice, sourceHeight, deviceScaleFactor), > > + FloatSize(rightSideScale, rightSideScale), Image::StretchTile, (Image::TileRule)vRule, ImagePaintingContext(op)); > > No need to change this. It should compile as is. Done. > > > Source/WebCore/rendering/RenderBoxModelObject.cpp:1373 > > graphicsContext->drawTiledImage(image.get(), colorSpace, pixelSnappedForPainting(x + leftWidth, y, destinationWidth, topWidth, deviceScaleFactor), > > - pixelSnappedForPainting(leftSlice, 0, sourceWidth, topSlice, deviceScaleFactor), FloatSize(topSideScale, topSideScale), (Image::TileRule)hRule, Image::StretchTile, op); > > + pixelSnappedForPainting(leftSlice, 0, sourceWidth, topSlice, deviceScaleFactor), FloatSize(topSideScale, topSideScale), > > + (Image::TileRule)hRule, Image::StretchTile, ImagePaintingContext(op)); > > No need to change this. It should compile as is. Done. > > > Source/WebCore/rendering/RenderBoxModelObject.cpp:1379 > > - graphicsContext->drawTiledImage(image.get(), colorSpace, pixelSnappedForPainting(x + leftWidth, borderImageRect.maxY() - bottomWidth, destinationWidth, bottomWidth, deviceScaleFactor), > > - pixelSnappedForPainting(leftSlice, imageHeight - bottomSlice, sourceWidth, bottomSlice, deviceScaleFactor), FloatSize(bottomSideScale, bottomSideScale), > > - (Image::TileRule)hRule, Image::StretchTile, op); > > + graphicsContext->drawTiledImage(image.get(), colorSpace, pixelSnappedForPainting(x + leftWidth, borderImageRect.maxY() - bottomWidth, destinationWidth, > > + bottomWidth, deviceScaleFactor), pixelSnappedForPainting(leftSlice, imageHeight - bottomSlice, sourceWidth, bottomSlice, deviceScaleFactor), > > + FloatSize(bottomSideScale, bottomSideScale), (Image::TileRule)hRule, Image::StretchTile, ImagePaintingContext(op)); > > No need to change this. It should compile as is. Done. > > > Source/WebCore/rendering/RenderBoxModelObject.cpp:1406 > > - middleScaleFactor, (Image::TileRule)hRule, (Image::TileRule)vRule, op); > > + middleScaleFactor, (Image::TileRule)hRule, (Image::TileRule)vRule, ImagePaintingContext(op)); > > No need to change this. It should compile as is. Done.
Comment on attachment 232362 [details] Patch Clearing flags on attachment: 232362 Committed r169529: <http://trac.webkit.org/changeset/169529>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 133447
Created attachment 232380 [details] Patch
Comment on attachment 232380 [details] Patch Missing paintingDisabled() check caused the assertion.
Comment on attachment 232380 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232380&action=review > Source/WebCore/platform/graphics/GraphicsContext.h:196 > + struct ImagePaintingContext { I don't really like this name; it sounds like it wraps a GraphicsContext. Maybe ImagePaintingOptions.
Comment on attachment 232380 [details] Patch Clearing flags on attachment: 232380 Committed r169531: <http://trac.webkit.org/changeset/169531>
(In reply to comment #14) > (From update of attachment 232380 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=232380&action=review > > > Source/WebCore/platform/graphics/GraphicsContext.h:196 > > + struct ImagePaintingContext { > > I don't really like this name; it sounds like it wraps a GraphicsContext. Maybe ImagePaintingOptions. https://trac.webkit.org/changeset/169534