[webkit-reviews] review granted: [Bug 191337] [Windows][DirectX] Update canvas code to pass more tests : [Attachment 354153] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Brent Fulgham <bfulgham at webkit.org>'s
request for review:
Bug 191337: [Windows][DirectX] Update canvas code to pass more tests
https://bugs.webkit.org/show_bug.cgi?id=191337

Attachment 354153: Patch

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




--- Comment #6 from Dean Jackson <dino at apple.com> ---
Comment on attachment 354153
  --> https://bugs.webkit.org/attachment.cgi?id=354153
Patch

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

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:703
> +    float miterLimit = 0.5f * canvasMiterLimit;

amazing

>> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:1560
>> +	flush();
> 
> Are all the `flush` calls necessary?

Yeah, why do you need to flush?

>
Source/WebCore/platform/graphics/win/GraphicsContextPlatformPrivateDirect2D.h:1
29
> +    float m_miterLimit { 10.0f };

Is this the same as other platforms? Why isn't this stored in GC.h?

> Source/WebCore/platform/graphics/win/ImageBufferDirect2D.cpp:203
> +    // FIME: See if we can reuse the on-hardware image

Typo: FIME and missing .

>> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:227
>> +	m_doesHaveOpenFigure = true;
> 
> Should this maybe just be `mutable`?

It seems to also be changing the active path values.

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:422
> +    float direction = (p2.x() - p1.x()) * (p0.y() - p1.y()) + (p2.y() -
p1.y()) * (p1.x() - p0.x());
> +    if (WTF::areEssentiallyEqual(direction, 0.0f))
> +	   return addLineTo(p1);

I don't think direction is the right name, but I can't think of a better one.
distance?

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:438
> +    bool anticlockwise = (direction < 0);

Oh. I see you use it as a direction here.

> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:242
> +#if USE(DIRECT2D)
> +std::unique_ptr<ImageBuffer> SVGRenderingContext::createImageBuffer(const
FloatRect& targetRect, const AffineTransform& absoluteTransform, ColorSpace
colorSpace, RenderingMode renderingMode, const GraphicsContext* context)
> +#else
>  std::unique_ptr<ImageBuffer> SVGRenderingContext::createImageBuffer(const
FloatRect& targetRect, const AffineTransform& absoluteTransform, ColorSpace
colorSpace, RenderingMode renderingMode)
> +#endif

I wonder if always passing the context and having an UNUSED_PARAM would be
better?