Bug 150049 – Add a CGContextStateSaver and use it

• Component: New Bugs
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Add a CGContextStateSaver and use it

[Simon Fraser (smfr)]

Created attachment 262924 [details]
Patch

[Myles C. Maxfield]

Why didn't you make GraphicsContextStateSaver use this internally?

[Simon Fraser (smfr)]

(In reply to comment #2)
> Why didn't you make GraphicsContextStateSaver use this internally?

Because that saves GraphicsContext, not CGContextState.

[Myles C. Maxfield]

(In reply to comment #3)
> (In reply to comment #2)
> > Why didn't you make GraphicsContextStateSaver use this internally?
> 
> Because that saves GraphicsContext, not CGContextState.

Surely saving a GraphicsContext involves saving CGContextState...

[Simon Fraser (smfr)]

https://trac.webkit.org/r190894

[Said Abou-Hallawa]

Comment on attachment 262924 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.h:86
> +};

Can't we write this class like this:

template <class T, void (*fnSave)(T), void (*fnRestore)(T)>
class ContextStateSaver
{
public:
    ContextStateSaver(T context, bool saveAndRestore = true)
        : m_context(context)
        , m_saveAndRestore(saveAndRestore)
    {
        if (m_saveAndRestore)
            fnSave(m_context);
    }

    ~ContextStateSaver()
    {
        if (m_saveAndRestore)
            fnRestore(m_context);
    }

    void save()
    {
        ASSERT(!m_saveAndRestore);
        fnRestore(m_context);
        m_saveAndRestore = true;
    }

    void restore()
    {
        ASSERT(m_saveAndRestore);
        fnRestore(m_context);
        m_saveAndRestore = false;
    }

private:
    T m_context;
    bool m_saveAndRestore;
};

typedef ContextStateSaver<CGContextRef, CGContextSaveGState, CGContextRestoreGState> CGContextStateSaver;

So if we want to do the same thing for any other port, we do not have to duplicate the code for the third time. Also GraphicsContextStateSaver can be a specialized case of this template, if we 

Implement these functions

    void GraphicsContextSaveGState(GraphicsContext& context) { context.save(); }
    void GraphicsContextRestoreGState(GraphicsContext& context) { context.restore(); }

And define 

    typedef ContextStateSaver<GraphicsContext&, GraphicsContextSaveGState, GraphicsContextRestoreGState> GraphicsContextStateSaver;

[Myles C. Maxfield]

Comment on attachment 262924 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.h:86
>> +};
> 
> Can't we write this class like this:
> 
> template <class T, void (*fnSave)(T), void (*fnRestore)(T)>
> class ContextStateSaver
> {
> public:
>     ContextStateSaver(T context, bool saveAndRestore = true)
>         : m_context(context)
>         , m_saveAndRestore(saveAndRestore)
>     {
>         if (m_saveAndRestore)
>             fnSave(m_context);
>     }
> 
>     ~ContextStateSaver()
>     {
>         if (m_saveAndRestore)
>             fnRestore(m_context);
>     }
> 
>     void save()
>     {
>         ASSERT(!m_saveAndRestore);
>         fnRestore(m_context);
>         m_saveAndRestore = true;
>     }
> 
>     void restore()
>     {
>         ASSERT(m_saveAndRestore);
>         fnRestore(m_context);
>         m_saveAndRestore = false;
>     }
> 
> private:
>     T m_context;
>     bool m_saveAndRestore;
> };
> 
> typedef ContextStateSaver<CGContextRef, CGContextSaveGState, CGContextRestoreGState> CGContextStateSaver;
> 
> So if we want to do the same thing for any other port, we do not have to duplicate the code for the third time. Also GraphicsContextStateSaver can be a specialized case of this template, if we 
> 
> Implement these functions
> 
>     void GraphicsContextSaveGState(GraphicsContext& context) { context.save(); }
>     void GraphicsContextRestoreGState(GraphicsContext& context) { context.restore(); }
> 
> And define 
> 
>     typedef ContextStateSaver<GraphicsContext&, GraphicsContextSaveGState, GraphicsContextRestoreGState> GraphicsContextStateSaver;

I think that currently this template stuff is more harm than good. However, if we add a single more class of this form, I would agree with Said.

[Simon Fraser (smfr)]

Comment on attachment 262924 [details]
Patch

I agree with Myles.

[Darin Adler]

Comment on attachment 262924 [details]
Patch

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

A couple thoughts about refinement.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.h:53
> +class CGContextStateSaver {

Nothing currently makes this class non-movable and non-copyable, and it should be. Could use the non-copyable macro or some other technique.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.h:55
> +    CGContextStateSaver(CGContextRef context, bool saveAndRestore = true)

Should mark constructor this explicit.

I don’t think using a boolean to tell the thing whether to save or not is a clear design. Can we do better?

[Said Abou-Hallawa]

Comment on attachment 262924 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.h:55
>> +    CGContextStateSaver(CGContextRef context, bool saveAndRestore = true)
> 
> Should mark constructor this explicit.
> 
> I don’t think using a boolean to tell the thing whether to save or not is a clear design. Can we do better?

I would suggest removing the boolean saveAndRestore from the constructor argument list but keeping m_saveAndRestore and initialize it always with true. I can't think of any scenario that I will do: CGContextStateSaver saver(context, false) and after that I call saver.save(). So I would suggest also removing the function save() from this class but keeping the restore() function.  The constructor will always save the context state and the destructor will restore it unless restore() was called before.

[Simon Fraser (smfr)]

(In reply to comment #10)
> Comment on attachment 262924 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262924&action=review
> 
> >> Source/WebCore/platform/graphics/cg/GraphicsContextCG.h:55
> >> +    CGContextStateSaver(CGContextRef context, bool saveAndRestore = true)
> > 
> > Should mark constructor this explicit.

Will fix.

> > I don’t think using a boolean to tell the thing whether to save or not is a clear design. Can we do better?

This actually happens a lot:

    bool drawOwnShadow = !isAcceleratedContext() && hasBlurredShadow() && !m_state.shadowsIgnoreTransforms; // Don't use ShadowBlur for canvas yet.
    CGContextStateSaver stateSaver(context, drawOwnShadow);
    if (drawOwnShadow) {

I did start with an enum argument, but that made these constructors all more ugly.

> I would suggest removing the boolean saveAndRestore from the constructor
> argument list but keeping m_saveAndRestore and initialize it always with
> true. I can't think of any scenario that I will do: CGContextStateSaver
> saver(context, false) and after that I call saver.save(). So I would suggest
> also removing the function save() from this class but keeping the restore()
> function.  The constructor will always save the context state and the
> destructor will restore it unless restore() was called before.

That sounds reasonable.