Bug 226170 – Virtualize GraphicsContext

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Virtualize GraphicsContext

[Tim Horton]

Created attachment 429514 [details]
Patch

[Tim Horton]

Not even close to ready for review, I just want to fiddle with EWS (and use it to get Windows building).

[Tim Horton]

Created attachment 429560 [details]
Patch

[Tim Horton]

Created attachment 429569 [details]
Patch

[Tim Horton]

Created attachment 429578 [details]
Patch

[Tim Horton]

Created attachment 429587 [details]
Patch

[Tim Horton]

Created attachment 429598 [details]
Patch

[Tim Horton]

Created attachment 429612 [details]
Patch

[Tim Horton]

Created attachment 429623 [details]
Patch

[Tim Horton]

Created attachment 429631 [details]
Patch

[Tim Horton]

Created attachment 429635 [details]
Patch

[Tim Horton]

Created attachment 429638 [details]
Patch

[Tim Horton]

Created attachment 429639 [details]
Patch

[Tim Horton]

Created attachment 429640 [details]
Patch

[Tim Horton]

Created attachment 429680 [details]
Patch

[Tim Horton]

Created attachment 429683 [details]
Patch

[Tim Horton]

Created attachment 429684 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 429680 [details]
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?

[Said Abou-Hallawa]

Comment on attachment 429684 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        In preparation for future patches which introduce new kinds of GraphicsContexts,
> +        virtualize GraphicsContext, moving platform implementations into subclasses
> +        GraphicsContext{CG,Cairo,Direct2D}, NullGraphicsContext, DisplayList::Recorder,
> +        and Nicosia::CairoOperationRecorder.

I think this is great work and it is going to save us from worrying about the paintingDisabled(), m_impl is not null, and the calling the platform function in very function of GraphicsContext. But I do not like the way the superclasses of GraphicsContext are created. I think they should be pointers from now on and the caller should only keep a std::unique_ptr< GraphicsContext > or RefPtr<GraphicsContext> instead. The creation can be the responsibility of GraphicsContext also. For example:

class GraphicsContext {
public:
    static std::unique_ptr< GraphicsContext > create(CGContextRef);

    static std::unique_ptr< GraphicsContext > create(PlatformContextCairo&);
    static std::unique_ptr< GraphicsContext > create(PlatformContextCairo*);
    static std::unique_ptr< GraphicsContext > create(cairo_t*);

    static std::unique_ptr< GraphicsContext > create(DisplayList&, const GraphicsContextState&, const FloatRect& initialClip, const AffineTransform&, Delegate*, DrawGlyphsRecorder::DrawGlyphsDeconstruction);
};

And this case a caller like the one in GraphicsLayerCA.cpp will do just this:

    auto context = GraphicsContext::create(*m_displayList, GraphicsContextState(), initialClip, AffineTransform());
    paintGraphicsLayerContents(*context, FloatRect(FloatPoint(), size()));

The reason for me preferring this approach over something like:

    GraphicsContextCG gc(context.get());

is we have to declare the variable to be GraphicsContextCG just to allocate the memory of the object correctly. But I do not think we will ever need to access the members the superclass directly. So using the the base type GraphicsContext is more appropriate and having it a pointer will solve the allocation issue.

[Tim Horton]

Created attachment 429698 [details]
Patch

[Tim Horton]

(In reply to Simon Fraser (smfr) from comment #18)
> Comment on attachment 429680 [details]
> 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

Leaving it for now.

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

Copy paste-o.

> > 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.

Both true.

> > Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h:53
> > +class Recorder : public GraphicsContext {
> 
> I think we should rename this class to RecordingContext

Strongly Agree. Leaving it for now.

> > 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?

Yes, done.

(In reply to Said Abou-Hallawa from comment #19)
> Comment on attachment 429684 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429684&action=review
> But I do not think we will ever need to
> access the members the superclass directly. So using the the base type
> GraphicsContext is more appropriate and having it a pointer will solve the
> allocation issue.

I think I agree with the premise (we don't need to call things on the subclass, and it would be nice if we just had a `GraphicsContext`). I don't want to de-stack-allocate all of the GraphicsContexts in this patch, though, that's a separate similarly large change :)

Also I don't think it's SO crazy to say `GraphicsContextCG` when *you are passing it a CGContextRef*. The code is already platform-specific.

[Tim Horton]

(Windows being the painfully obvious counterpoint to my last point there)

[EWS]

Committed r278062 (238144@main): <https://commits.webkit.org/238144@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429698 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78483981>