Add a CGContextStateSaver and use it
Created attachment 262924 [details] Patch
Why didn't you make GraphicsContextStateSaver use this internally?
(In reply to comment #2) > Why didn't you make GraphicsContextStateSaver use this internally? Because that saves GraphicsContext, not CGContextState.
(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...
https://trac.webkit.org/r190894
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;
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.
Comment on attachment 262924 [details] Patch I agree with Myles.
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?
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.
(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.