[webkit-reviews] review denied: [Bug 49914] Merge GraphicsContextPrivate into GraphicsContext : [Attachment 75737] Patch

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has denied Renata Hodovan <reni at webkit.org>'s
request for review:
Bug 49914: Merge GraphicsContextPrivate into GraphicsContext
https://bugs.webkit.org/show_bug.cgi?id=49914

Attachment 75737: Patch
https://bugs.webkit.org/attachment.cgi?id=75737&action=review

------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=75737&action=review

Getting better, but not quite there yet. :)

First and foremost your copy of GraphicsContextState appears to be out-of-date,
smfr reordered the members in <http://trac.webkit.org/changeset/73292>

> WebCore/platform/graphics/GraphicsContext.cpp:102
> +	   LOG_ERROR("ERROR void GraphicsContext::restore() m_stack is empty");


Nit: I suspect that "stack" here refers to the state stack, not the actual
variable name.

> WebCore/platform/graphics/GraphicsContext.h:206
> +	   virtual ~GraphicsContext();
> +
> +	   virtual void platformInit(PlatformGraphicsContext*);
> +	   virtual void platformDestroy();

These should not be virtual! Nothing inherits from GraphicsContext.

> WebCore/platform/graphics/GraphicsContext.h:243
> +	   GraphicsContextState state() const;

This should return a 'const GraphicsContextState&' instead of a copy.

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:202
> -GraphicsContext::GraphicsContext(PlatformGraphicsContext* cr)
> -    : m_common(createGraphicsContextPrivate())
> -    , m_data(new GraphicsContextPlatformPrivate)
> +void GraphicsContext::platformInit(PlatformGraphicsContext* cr)
>  {
> +    m_data = new GraphicsContextPlatformPrivate;

Why are you switching away from using initializer list syntax for m_data?
(This comment repeated for all GraphicsContext implementations.)