[webkit-reviews] review requested: [Bug 49914] GraphicsContext: Merge m_common and m_data : [Attachment 75155] Patch

[bugzilla-daemon at webkit.org]

Balazs Kelemen <kbalazs at webkit.org> has asked  for review:
Bug 49914: GraphicsContext: Merge m_common and m_data
https://bugs.webkit.org/show_bug.cgi?id=49914

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

------- Additional Comments from Balazs Kelemen <kbalazs at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=75155&action=review

Generally the patch would do it's job correctly, but I am doubtful about what
do we win with that.
As I see a renaming from GraphicsContextPrivate to GraphicsContextState and
m_common to m_state would be
enough to make the roles of those member clear.  Could you give some arguments
of the change in the ChangeLog?

> WebCore/platform/graphics/GraphicsContext.h:-448
>	   GraphicsContextPrivate* m_common;
> -	   GraphicsContextPlatformPrivate* m_data; // Deprecated; m_commmon can
just be downcasted. To be removed.

I think m_common should be renamed to m_data.

> WebCore/platform/graphics/cg/GraphicsContextCG.cpp:114
>  }
>  
>  GraphicsContext::GraphicsContext(CGContextRef cgContext)
> -    : m_common(createGraphicsContextPrivate())
> -    , m_data(new GraphicsContextPlatformPrivate(cgContext))
> +    : m_common(new GraphicsContextPlatformPrivate(cgContext))
>  {
>      setPaintingDisabled(!cgContext);
>      if (cgContext) {

After this change nobody should ever create a GraphicsContextPrivate object so
it's constructor should be private (with a comment maybe).

> WebCore/platform/graphics/haiku/GraphicsContextHaiku.cpp:173
> +    rgb_color oldColor = platformPrivate()->m_view->HighColor();
> +    platformPrivate()->m_view->SetHighColor(color);
> +    platformPrivate()->m_view->FillRect(rect);
> +    platformPrivate()->m_view->SetHighColor(oldColor);

platformPrivate() should be assigned to a local variable. I am not sure about
what is the preferred style, but maybe you should do it in every case when 
you call a getter more than once in a function. Personally I am OK with calling
it twice (but not more) because it is just an inline getter.

> WebCore/platform/graphics/haiku/GraphicsContextHaiku.cpp:296
> +    float oldSize = platformPrivate()->m_view->PenSize();
> +    platformPrivate()->m_view->SetPenSize(width);
> +    platformPrivate()->m_view->StrokeRect(rect, getHaikuStrokeStyle());
> +    platformPrivate()->m_view->SetPenSize(oldSize);

Ditto.

> WebCore/platform/graphics/haiku/GraphicsContextHaiku.cpp:317
> -    m_data->m_view->SetLineMode(mode, m_data->m_view->LineJoinMode(),
m_data->m_view->LineMiterLimit());
> +    platformPrivate()->m_view->SetLineMode(mode,
platformPrivate()->m_view->LineJoinMode(),
platformPrivate()->m_view->LineMiterLimit());

Ditto. I will not repeat that observation in the rest of the patch.