[webkit-reviews] review granted: [Bug 209635] Hovering over countries at https://covidinc.io/ shows bizarre rendering artifacts : [Attachment 394732] Patch

[bugzilla-daemon at webkit.org]

Said Abou-Hallawa <sabouhallawa at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 209635: Hovering over countries at https://covidinc.io/ shows bizarre
rendering artifacts
https://bugs.webkit.org/show_bug.cgi?id=209635

Attachment 394732: Patch

https://bugs.webkit.org/attachment.cgi?id=394732&action=review




--- Comment #6 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
Comment on attachment 394732
  --> https://bugs.webkit.org/attachment.cgi?id=394732
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394732&action=review

> Source/WebCore/ChangeLog:10
> +	   when using a image buffer mask. However, it created and rendered
into this image buffer

However, it is created...?

> Source/WebCore/rendering/RenderLayer.cpp:4497
> +	       auto referenceBox =
snapRectToDevicePixels(rootRelativeBounds.value(),
renderer().document().deviceScaleFactor());

The description mentioned the ImageBuffer wrong caching so I am confused about
this change. Is this cleanup or it is part of this fix?

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:131
>  bool RenderSVGResourceClipper::applyClippingToContext(RenderElement&
renderer, const FloatRect& objectBoundingBox, const FloatRect& repaintRect,
GraphicsContext& context)

I think we can get rid of repaintRect. It is only used in the LOG_WITH_STREAM
statement and in the if-statement which recreates the ImageBuffer. I think the
caller should avoid calling this function if the repaintRect.isEmpty().

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:146
> +    auto canUseExistingImageBuffer = [&]() {
> +	   return clipperData.imageBuffer && clipperData.objectBoundingBox ==
objectBoundingBox && clipperData.absoluteTransform == absoluteTransform;
> +    };

Should not be this a function of ClipperData?

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:156
> +	   clipperData.imageBuffer = WTFMove(maskImage);
> +	   clipperData.objectBoundingBox = objectBoundingBox;
> +	   clipperData.absoluteTransform = absoluteTransform;

Can't this code be a constructor of ClipperData?
    clipperData = { WTFMove(maskImage), objectBoundingBox, absoluteTransform };

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:177
> +	       clipperData.imageBuffer = nullptr;

Should not we clear everything, i.e. clipperData = { };?

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:187
> +bool RenderSVGResourceClipper::drawContentIntoMaskImage(ImageBuffer&
maskImageBuffer, const FloatRect& objectBoundingBox)

I think maskImageBuffer should be const ImageBuffer&.

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:239
> -	  
SVGRenderingContext::renderSubtreeToImageBuffer(clipperMaskImage.get(),
isUseElement ? *child.renderer() : *renderer, maskContentTransformation);
> +	   SVGRenderingContext::renderSubtreeToImageBuffer(&maskImageBuffer,
isUseElement ? *child.renderer() : *renderer, maskContentTransformation);

Can't we just pass the maskImageBuffer->context() instead of maskImageBuffer
since renderSubtreeToImageBuffer() is using the ImageBuffer to get its
context()?

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.h:33
> -typedef std::unique_ptr<ImageBuffer> ClipperMaskImage;
> +using ClipperMaskImage = std::unique_ptr<ImageBuffer>;

Do we still need this? It seems you replaced all the reference to
ClipperMaskImage by either ImageBuffer or ClipperData.