Bug 162923 – [Win][Direct2D] Add initial D2D Path and Gradient implementation

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

[Brent Fulgham]

This patch adds a Direct2D Path implementation, sufficient to support basic HTML5 Canvas operations.

This patch simply adds the new files. It does not incorporate them into the build.

[Brent Fulgham]

Created attachment 290670 [details]
Patch

[Alex Christensen]

Comment on attachment 290670 [details]
Patch

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

lots of relatively small things.  r=me once they're addressed.

> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:70
> +            D2D1::RadialGradientBrushProperties(p0(), D2D1::Point2F(0, 0) /*p1()*/, startRadius(), endRadius()),

Is there a way we could not commit strangely-commented-out code like this?
// FIXME: Replace the second parameter with p1().

> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:116
> +    d2dContext->SetTags(3, 103);

These magic numbers need names and explanations.

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:212
> +    (void)applier;
> +    (void)point;

Please use UNUSED_PARAM

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:261
> +        return FloatRect();

{ } here and elsewhere

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:288
> +    // FIXME
> +    notImplemented();

FIXME not needed.

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:335
> +    (void)p1;
> +    (void)p2;
> +    (void)radius;

UNUSED_PARAM, or just don't name the parameters.

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:342
> +    bool equalWidths = (topLeftRadius.width() == topRightRadius.width() && topRightRadius.width() == bottomLeftRadius.width() && bottomLeftRadius.width() == bottomRightRadius.width());
> +    bool equalHeights = (topLeftRadius.height() == bottomLeftRadius.height() && bottomLeftRadius.height() == topRightRadius.height() && topRightRadius.height() == bottomRightRadius.height());

These could use a helper function.

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:366
> +    m_activePath->EndFigure(D2D1_FIGURE_END_OPEN /*D2D1_FIGURE_END_CLOSED*/);

This shouldn't be committed like this.  Pick one.  Use a FIXME if we intend to change something.  Use a 2-state enum parameter if we want to be able to choose.

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:403
> +    if (std::abs(endAngle - firstArcEnd) < 0.001)

areEssentiallyEqual, and make epsilon smaller if possible.

[Brent Fulgham]

Comment on attachment 290670 [details]
Patch

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

Thanks for reviewing this. Maybe soon I can get you to hack on it ... :-)

>> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:70
>> +            D2D1::RadialGradientBrushProperties(p0(), D2D1::Point2F(0, 0) /*p1()*/, startRadius(), endRadius()),
> 
> Is there a way we could not commit strangely-commented-out code like this?
> // FIXME: Replace the second parameter with p1().

That should just be p1(). Thanks for catching that -- I was debugging the radial gradient code and forget I had set p1 to origin.

>> Source/WebCore/platform/graphics/win/GradientDirect2D.cpp:116
>> +    d2dContext->SetTags(3, 103);
> 
> These magic numbers need names and explanations.

This is a D2D debugging function that takes two numbers as input. When you flush the context, if there is an error it gives the most recent tags (so you can locate the drawing commands that were in place when the failure occurred).

I switched to using "__LINE__" for the second argument and missed that in this file.

I'll add a macro for the first number, too.

>> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:212
>> +    (void)point;
> 
> Please use UNUSED_PARAM

Done!

>> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:261
>> +        return FloatRect();
> 
> { } here and elsewhere

Done!

>> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:288
>> +    notImplemented();
> 
> FIXME not needed.

OK!

>> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:335
>> +    (void)radius;
> 
> UNUSED_PARAM, or just don't name the parameters.

Done!

>> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:342
>> +    bool equalHeights = (topLeftRadius.height() == bottomLeftRadius.height() && bottomLeftRadius.height() == topRightRadius.height() && topRightRadius.height() == bottomRightRadius.height());
> 
> These could use a helper function.

Good idea. Done.

>> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:366
>> +    m_activePath->EndFigure(D2D1_FIGURE_END_OPEN /*D2D1_FIGURE_END_CLOSED*/);
> 
> This shouldn't be committed like this.  Pick one.  Use a FIXME if we intend to change something.  Use a 2-state enum parameter if we want to be able to choose.

The commented version was not needed. It's gone.

>> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:403
>> +    if (std::abs(endAngle - firstArcEnd) < 0.001)
> 
> areEssentiallyEqual, and make epsilon smaller if possible.

Yes! Done.

[Brent Fulgham]

Committed r206815: <http://trac.webkit.org/changeset/206815>