Bug 191452 – [Windows][DirectX] Be more rigorous about BeginFigure/EndFigure and Close operations

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

[Brent Fulgham]

The ID2D1GeometrySink used to represent segments of an open Path require that all cases of BeginFigure be matched with a corresponding EndFigure. If the BeginFigure/EndFigure counts do not match when the sink is closed, the sink will enter an error state and will not draw content.

This patch keeps track of the BeginFigure/EndFigure operations, and ensures the calls are in balance before closing.

[Radar WebKit Bug Importer]

<rdar://problem/45933964>

[Brent Fulgham]

Created attachment 354314 [details]
Patch

[Brent Fulgham]

Created attachment 354315 [details]
Patch

[Brent Fulgham]

This moves us up to 580/767.

[WebKit Commit Bot]

Comment on attachment 354315 [details]
Patch

Clearing flags on attachment: 354315

Committed r238037: <https://trac.webkit.org/changeset/238037>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Checked in as "rigors"

[Said Abou-Hallawa]

Comment on attachment 354315 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +      
> +

An extra line here.

> Source/WebCore/platform/graphics/Path.h:216
> +        size_t m_openFigureCount { 0 };

Why it is size_t? Should not it be just unsigned?

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:227
>      m_activePath->BeginFigure(currentPoint, D2D1_FIGURE_BEGIN_FILLED);
> -    m_doesHaveOpenFigure = true;
> +    ++m_openFigureCount;

This makes the code a little hard to read. closeAnyOpenGeometries() hides the cumbersomeness of m_openFigureCount. But here and every call to BeginFigure, you have to increment m_openFigureCount. Why do not we add a function for opening the figure similar to Path::closeAnyOpenGeometries().

void Path::openFigure(const FloatPoint& point)
{
    m_activePath->BeginFigure(point, D2D1_FIGURE_BEGIN_FILLED);
   ++m_openFigureCount;
}

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:277
> +void Path::closeAnyOpenGeometries()
> +{
> +    ASSERT(m_activePath);
> +
> +    if (!m_openFigureCount)
> +        return;
> +
> +    while (m_openFigureCount) {
> +        m_activePath->EndFigure(D2D1_FIGURE_END_OPEN);
> +        --m_openFigureCount;
> +    }
> +
> +    HRESULT hr = m_activePath->Close();
> +    ASSERT(SUCCEEDED(hr));
> +}

I think this function does more than its name says. It closes all the opened figures and closes m_activePath. I would suggest naming it Path::closePath(). I noticed there is a function named Path::closeSubpath() which ends one figure and closes m_activePath.

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:297
>          m_activePath = nullptr;
>          m_activePathGeometry = nullptr;

Why do not we move these assignments to closeAnyOpenGeometries()?

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:508
> +    if (!m_activePath) {
> +        ASSERT(!m_openFigureCount);
> +        ASSERT(!m_activePathGeometry);
> +        return;
> +    }

These kind of assertions/dependencies make me feel these data members should be encapsulated in one struct or class.