Bug 191337 – [Windows][DirectX] Update canvas code to pass more tests

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brent Fulgham]

The changes in this patch address a number of test failures in the canvas/philip test suite.

[Radar WebKit Bug Importer]

<rdar://problem/45878801>

[Brent Fulgham]

Created attachment 354149 [details]
Patch

[Brent Fulgham]

Created attachment 354153 [details]
Patch

[Brent Fulgham]

Would it be okay to just thread an Optional<GraphicsContext> or GraphicsContext* through SVGRenderingContext?

[Don Olmstead]

Comment on attachment 354153 [details]
Patch

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

Seems fine. Just a few nits.

Hows this doing on the tests?

> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:1560
> +    flush();

Are all the `flush` calls necessary?

> Source/WebCore/platform/graphics/win/ImageBufferDirect2D.cpp:166
> +        /*auto context = adoptCF(CGBitmapContextCreate(0, logicalSize.width(), logicalSize.height(), 8, 4 * logicalSize.width(), sRGBColorSpaceRef(), kCGImageAlphaPremultipliedLast));
> +        CGContextSetBlendMode(context.get(), kCGBlendModeCopy);
> +        CGContextClipToRect(context.get(), FloatRect(FloatPoint::zero(), logicalSize));
> +        FloatSize imageSizeInUserSpace = scaleSizeToUserSpace(logicalSize, backingStoreSize, internalSize);
> +        CGContextDrawImage(context.get(), FloatRect(FloatPoint::zero(), imageSizeInUserSpace), image.get());
> +        image = adoptCF(CGBitmapContextCreateImage(context.get()));*/

This is commented out

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:227
> +    m_doesHaveOpenFigure = true;

Should this maybe just be `mutable`?

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:590
> +    HRESULT hr = GraphicsContext::systemFactory()->CreateEllipseGeometry(D2D1::Ellipse(r.center(), 0.5 * r.width(), 0.5 * r.height()), &ellipse);

Comment about the 0.5?

[Dean Jackson]

Comment on attachment 354153 [details]
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:129
> +    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?

[Brent Fulgham]

Comment on attachment 354153 [details]
Patch

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

>>> Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp:1560
>>> +    flush();
>> 
>> Are all the `flush` calls necessary?
> 
> Yeah, why do you need to flush?

Maybe not! I needed it for stroking and filling rects, but clipping probably no. I'll remove it.

>> Source/WebCore/platform/graphics/win/GraphicsContextPlatformPrivateDirect2D.h:129
>> +    float m_miterLimit { 10.0f };
> 
> Is this the same as other platforms? Why isn't this stored in GC.h?

Amazingly, this is also 10.0 for CG (in the headers). I'm not sure why it's missing from GraphicsContext.h.

>> Source/WebCore/platform/graphics/win/ImageBufferDirect2D.cpp:166
>> +        image = adoptCF(CGBitmapContextCreateImage(context.get()));*/
> 
> This is commented out

Whoops. I was looking at the CG code to implement and didn't port it over. I'll remove this and make it 'notImplemented()'.

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

Will fi.

>>> 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.

Right -- it touches a few things. My original version of this should never have been const :-\.

>> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:590
>> +    HRESULT hr = GraphicsContext::systemFactory()->CreateEllipseGeometry(D2D1::Ellipse(r.center(), 0.5 * r.width(), 0.5 * r.height()), &ellipse);
> 
> Comment about the 0.5?

Will do.

>> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:242
>> +#endif
> 
> I wonder if always passing the context and having an UNUSED_PARAM would be better?

Yes -- I had this originally but worried it would mess something up. I'll revise the patch to do so and do a land-safely to make sure it works.

[Brent Fulgham]

Created attachment 354187 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 354187 [details]
Patch for landing

Rejecting attachment 354187 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 354187, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=354187&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=191337&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 354187 from bug 191337.
Fetching: https://bugs.webkit.org/attachment.cgi?id=354187
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 14 diffs from patch file(s).
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/platform/graphics/ImageBuffer.cpp
patching file Source/WebCore/platform/graphics/ImageBuffer.h
patching file Source/WebCore/platform/graphics/Path.h
patching file Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp
Hunk #10 FAILED at 1697.
1 out of 10 hunks FAILED -- saving rejects to file Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp.rej
patching file Source/WebCore/platform/graphics/win/GraphicsContextPlatformPrivateDirect2D.h
patching file Source/WebCore/platform/graphics/win/ImageBufferDirect2D.cpp
patching file Source/WebCore/platform/graphics/win/PathDirect2D.cpp
patching file Source/WebCore/platform/graphics/win/SimpleFontDataDirect2D.cpp
patching file Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp
patching file Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp
patching file Source/WebCore/rendering/svg/RenderSVGResourceMasker.cpp
patching file Source/WebCore/rendering/svg/SVGRenderingContext.cpp
patching file Source/WebCore/rendering/svg/SVGRenderingContext.h

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/9902055

[Brent Fulgham]

Committed r237977: <https://trac.webkit.org/changeset/237977>