Bug 191452 - [Windows][DirectX] Be more rigorous about BeginFigure/EndFigure and Close operations
Summary: [Windows][DirectX] Be more rigorous about BeginFigure/EndFigure and Close ope...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks: 191411
  Show dependency treegraph
 
Reported: 2018-11-08 20:35 PST by Brent Fulgham
Modified: 2018-11-09 10:45 PST (History)
8 users (show)

See Also:


Attachments
Patch (5.55 KB, patch)
2018-11-08 21:45 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (7.54 KB, patch)
2018-11-08 22:13 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2018-11-08 20:35:05 PST
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.
Comment 1 Radar WebKit Bug Importer 2018-11-08 20:37:41 PST
<rdar://problem/45933964>
Comment 2 Brent Fulgham 2018-11-08 21:45:42 PST
Created attachment 354314 [details]
Patch
Comment 3 Brent Fulgham 2018-11-08 22:13:59 PST
Created attachment 354315 [details]
Patch
Comment 4 Brent Fulgham 2018-11-08 22:19:24 PST
This moves us up to 580/767.
Comment 5 WebKit Commit Bot 2018-11-09 09:32:39 PST
Comment on attachment 354315 [details]
Patch

Clearing flags on attachment: 354315

Committed r238037: <https://trac.webkit.org/changeset/238037>
Comment 6 WebKit Commit Bot 2018-11-09 09:32:41 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Simon Fraser (smfr) 2018-11-09 10:03:35 PST
Checked in as "rigors"
Comment 8 Said Abou-Hallawa 2018-11-09 10:45:02 PST
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.