[webkit-reviews] review granted: [Bug 240363] Simplify the usage of DrawGlyphsRecorder : [Attachment 459258] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 240363: Simplify the usage of DrawGlyphsRecorder
https://bugs.webkit.org/show_bug.cgi?id=240363

Attachment 459258: Patch

https://bugs.webkit.org/attachment.cgi?id=459258&action=review




--- Comment #3 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 459258
  --> https://bugs.webkit.org/attachment.cgi?id=459258
Patch

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

> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:168
> +	   if (!m_drawGlyphsRecorder)
> +	       m_drawGlyphsRecorder = makeUnique<DrawGlyphsRecorder>(*this,
m_initialScale);

I still think an "ensure" function would make this more readable.

> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h:50
> +enum class DeconstructDrawGlyphs : bool { No, Yes };

I generally like to put these things inside classes to not pollute the global
namespace. This type is only meaningful inside the class anyway.

> Source/WebCore/platform/graphics/displaylists/DisplayListRecorderImpl.h:38
> -    WEBCORE_EXPORT RecorderImpl(DisplayList&, const GraphicsContextState&,
const FloatRect& initialClip, const AffineTransform&,
DrawGlyphsRecorder::DeconstructDrawGlyphs =
DrawGlyphsRecorder::DeconstructDrawGlyphs::Yes);
> +    WEBCORE_EXPORT RecorderImpl(DisplayList&, const GraphicsContextState&,
const FloatRect& initialClip, const AffineTransform&, DeconstructDrawGlyphs =
DeconstructDrawGlyphs::No);

Changing the default value is suspicious. Surely the default option should be
the one which is more secure, right?

> Source/WebKit/WebProcess/GPU/graphics/RemoteDisplayListRecorderProxy.cpp:53
> +    ASSERT(deconstructDrawGlyphs ==
DisplayList::DeconstructDrawGlyphs::Yes);

Why not just remove the parameter then?