Bug 162923 - [Win][Direct2D] Add initial D2D Path and Gradient implementation
Summary: [Win][Direct2D] Add initial D2D Path and Gradient implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: PC All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords:
Depends on:
Blocks: 161817
  Show dependency treegraph
 
Reported: 2016-10-04 11:04 PDT by Brent Fulgham
Modified: 2016-10-05 10:20 PDT (History)
4 users (show)

See Also:


Attachments
Patch (24.91 KB, patch)
2016-10-04 16:58 PDT, Brent Fulgham
achristensen: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2016-10-04 11:04:39 PDT
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.
Comment 1 Brent Fulgham 2016-10-04 16:58:30 PDT
Created attachment 290670 [details]
Patch
Comment 2 Alex Christensen 2016-10-05 00:17:07 PDT
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.
Comment 3 Brent Fulgham 2016-10-05 10:20:27 PDT
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.
Comment 4 Brent Fulgham 2016-10-05 10:20:59 PDT
Committed r206815: <http://trac.webkit.org/changeset/206815>