[webkit-reviews] review granted: [Bug 226170] Virtualize GraphicsContext : [Attachment 429680] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted  review:
Bug 226170: Virtualize GraphicsContext
https://bugs.webkit.org/show_bug.cgi?id=226170

Attachment 429680: Patch

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




--- Comment #18 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 429680
  --> https://bugs.webkit.org/attachment.cgi?id=429680
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext.h:341
> +    void setDrawLuminanceMask(bool drawLuminanceMask) {
m_state.drawLuminanceMask = drawLuminanceMask; updateState(m_state,
GraphicsContextState::DrawLuminanceMaskChange); }
> +    bool drawLuminanceMask() const { return m_state.drawLuminanceMask; }

Maybe we could rename these to reduce ambiguity:
setIsDrawingLuminanceMask/isDrawingLuminanceMask

> Source/WebCore/platform/graphics/NullGraphicsContext.h:52
> +    bool paintingDisabled() const override { return true; }

Why not final?

> Source/WebCore/platform/graphics/NullGraphicsContext.h:155
> +    const PaintInvalidationReasons m_paintInvalidationReasons {
PaintInvalidationReasons::None };

This could be an OptionSet<> in future.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:105
> +    bool fill = mode.contains(TextDrawingMode::Fill);
> +    bool stroke = mode.contains(TextDrawingMode::Stroke);
> +    if (fill && stroke)
> +	   return kCGTextFillStroke;
> +    if (fill)
> +	   return kCGTextFill;
> +    return kCGTextStroke;

Could be simplified with some OptionSet<> goop.

> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h:53
> +class Recorder : public GraphicsContext {

I think we should rename this class to RecordingContext

> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h:95
> +#if USE(CG) || USE(DIRECT2D)
> +    void setIsCALayerContext(bool) override { }
> +    bool isCALayerContext() const override { return false; }
> +    void setIsAcceleratedContext(bool) override { }
> +#endif
> +
> +    void fillRoundedRectImpl(const FloatRoundedRect&, const Color&) override
{ ASSERT_NOT_REACHED(); }
> +    void drawLineForText(const FloatRect&, bool, bool, StrokeStyle) override
{ ASSERT_NOT_REACHED(); }

Can everything be final here?