Bug 201534 – PathDirect2D Cleanups

• Component: WebKit API
• Status: RESOLVED FIXED

[Brent Fulgham]

The initial PathDirect2D implementation had a few shortcomings:

1. COMPtr<> had no move operator, therefore the Path implementation couldn't move content.
2. Paths (ID2D1GeometryGroups) are frequently built up of ID2D1Geometry elements. But the append operation required retrieving the existing geometries, adding the new item, then loading back into an overall path object. By keeping track of the elements ourselves, we can avoid copying the elements into a temporary vector before building the appended ID2D1GeometryGroup.
3. There are rules about using ID2D1GeometrySink and ID2D1PathGeometry. After calling 'Close' on an ID2D1GeometrySink supplied by a ID2D1PathGeometry, neither should be reused. Nothing prevented this mistake in the PathDirect2D implementation.

This patch cleans up these issues.

[Brent Fulgham]

Created attachment 378158 [details]
Patch

[Brent Fulgham]

Created attachment 378159 [details]
Patch

[Fujii Hironori]

Comment on attachment 378159 [details]
Patch

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

> Source/WebCore/platform/graphics/Path.h:199
>          void drawDidComplete();

Path::drawDidComplete is not used anywhere in WebKit. It should be removed.

> Source/WebCore/platform/graphics/Path.h:215
>          COMPtr<ID2D1PathGeometry> m_activePathGeometry;

m_activePathGeometry is not needed because it is stored in m_geometries and the active path sync is stored in m_activePath.

> Source/WebCore/platform/graphics/win/Direct2DOperations.cpp:498
> +    const_cast<Path&>(path).closeAnyOpenGeometries(D2D1_FIGURE_END_OPEN);

Can you remove `const_cast` by making closeAnyOpenGeometries a mutable method?

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:118
> +    HRESULT hr = GraphicsContext::systemFactory()->CreateGeometryGroup(fillMode, m_geometries.data(), m_geometries.size(), &m_path);

Path::appendGeometry always create a GeometryGroup by calling CreateGeometryGroup.
Can it be done in Path::closeAnyOpenGeometries?

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:311
> +        while (m_openFigureCount) {

m_openFigureCount can be 1 or 0. m_openFigureCount can be bool m_isFigureOpened.

[Fujii Hironori]

Comment on attachment 378159 [details]
Patch

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

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:343
> +        closeAnyOpenGeometries(D2D1_FIGURE_END_OPEN);

You are trying to keep a active path still active even after transforming.
Is this really needed? If so, this is not a right way to do it.
You close the active path with D2D1_FIGURE_END_OPEN, and create a new one.

[Brent Fulgham]

Created attachment 378205 [details]
Patch

[Fujii Hironori]

Comment on attachment 378205 [details]
Patch

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

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:-220
> -    HRESULT hr = const_cast<Path*>(this)->initializePathState();

Path::initializePathState becomes unused. Remove it.

> Source/WebCore/platform/win/COMPtr.h:175
> +        ptr->Release();

There is no reason to defer calling Release after assignment. Call Release before the assignment.

[Brent Fulgham]

Comment on attachment 378205 [details]
Patch

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

>> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:-220
>> -    HRESULT hr = const_cast<Path*>(this)->initializePathState();
> 
> Path::initializePathState becomes unused. Remove it.

Oh, so it does! Removed.

>> Source/WebCore/platform/win/COMPtr.h:175
>> +        ptr->Release();
> 
> There is no reason to defer calling Release after assignment. Call Release before the assignment.

Will do.

[Brent Fulgham]

Created attachment 378250 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 378250 [details]
Patch

Clearing flags on attachment: 378250

Committed r249604: <https://trac.webkit.org/changeset/249604>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55137930>

[Radar WebKit Bug Importer]

<rdar://problem/55137931>