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.
<rdar://problem/45933964>
Created attachment 354314 [details] Patch
Created attachment 354315 [details] Patch
This moves us up to 580/767.
Comment on attachment 354315 [details] Patch Clearing flags on attachment: 354315 Committed r238037: <https://trac.webkit.org/changeset/238037>
All reviewed patches have been landed. Closing bug.
Checked in as "rigors"
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.