[webkit-reviews] review granted: [Bug 133434] Provide better encapsulation for image related painting properties. : [Attachment 232354] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Zalan Bujtas <zalan at apple.com>'s
request for review:
Bug 133434: Provide better encapsulation for image related painting properties.
https://bugs.webkit.org/show_bug.cgi?id=133434

Attachment 232354: Patch
https://bugs.webkit.org/attachment.cgi?id=232354&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.