Bug 49914 – Merge GraphicsContextPrivate into GraphicsContext

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Andreas Kling]

From GraphicsContext.h:

GraphicsContextPrivate* m_common;
GraphicsContextPlatformPrivate* m_data; // Deprecated; m_commmon can just be downcasted. To be removed.

[Renata Hodovan]

Created attachment 75155 [details]
Patch

[Andreas Kling]

Comment on attachment 75155 [details]
Patch

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

> WebCore/platform/graphics/GraphicsContext.h:445
>          static GraphicsContextPrivate* createGraphicsContextPrivate();
>          static void destroyGraphicsContextPrivate(GraphicsContextPrivate*);

These two methods should be removed since they are no longer used/needed.

> WebCore/platform/graphics/wx/GraphicsContextWx.cpp:139
>      destroyGraphicsContextPrivate(m_common);
> -    delete m_data;
> +    delete platformPrivate();

This will crash! (Double delete)

[Eric Seidel (no email)]

Attachment 75155 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6461021

[WebKit Review Bot]

Attachment 75155 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/6328116

[Balazs Kelemen]

Comment on attachment 75155 [details]
Patch

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.

[Andreas Kling]

(In reply to comment #5)
> 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.

GraphicsContextPrivate has a member "GraphicsContextPrivate::GraphicsContextState state" so this would be rather confusing, e.g "m_state->state"

> I think m_common should be renamed to m_data.

Agreed.

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

I don't know if there's a preferred style, but I agree that repeating platformPrivate() all over the place is rather ugly. :)

[Andreas Kling]

Comment on attachment 75155 [details]
Patch

Resetting r- as the patch has numerous issues. (Guess the re-r? was accidental.)

[Balazs Kelemen]

(In reply to comment #6)
> (In reply to comment #5)
> > 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.
> 
> GraphicsContextPrivate has a member "GraphicsContextPrivate::GraphicsContextState state" so this would be rather confusing, e.g "m_state->state"

That's true but the point is that we are trying to merge two different
things into one class. Actually GraphicsContextPrivate is just data (no methods)
so we could simply merge it into GraphicsContext and let GraphicsContextPlatformPrivate
as it is.

[Andreas Kling]

(In reply to comment #8)
> That's true but the point is that we are trying to merge two different
> things into one class. Actually GraphicsContextPrivate is just data (no methods)
> so we could simply merge it into GraphicsContext and let GraphicsContextPlatformPrivate
> as it is.

You are absolutely right, this would be a lot cleaner. :)

[Renata Hodovan]

Created attachment 75370 [details]
Patch

This patch follows the previous suggestions. I merged all data from GraphicsContextPrivate into GraphicsContext and removed the m_common member.

[Balazs Kelemen]

Comment on attachment 75370 [details]
Patch

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

Good patch with not so good Changelog.

> WebCore/ChangeLog:9
> +        GraphicsContext: Merge m_common and m_data
> +        https://bugs.webkit.org/show_bug.cgi?id=49914
> +
> +        Move data members from GraphicsContextPrivate into GraphicsContext. So the m_common member became unnecessary.
> +        It is removed.

Why? I know but the Changelog should contain arguments for the change. Don't sparing with the words in Changelog's! :)

[Andreas Kling]

Comment on attachment 75370 [details]
Patch

Removing r? since Reni is working on a new version of this patch.

[Renata Hodovan]

Created attachment 75729 [details]
Patch

[Renata Hodovan]

Created attachment 75737 [details]
Patch

[Build Bot]

Attachment 75737 [details] did not build on win:
Build output: http://queues.webkit.org/results/6851058

[Andreas Kling]

(In reply to comment #15)
> Attachment 75737 [details] did not build on win:
> Build output: http://queues.webkit.org/results/6851058

3>..\platform\graphics\win\GraphicsContextCGWin.cpp(65) : error C2511: 'void WebCore::GraphicsContext::platformInit(HDC,bool)' : overloaded member function not found in 'WebCore::GraphicsContext'
3>        c:\cygwin\home\buildbot\WebKit\WebCore\platform\graphics\GraphicsContext.h(200) : see declaration of 'WebCore::GraphicsContext'

[Andreas Kling]

Comment on attachment 75737 [details]
Patch

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

[Andreas Kling]

(In reply to comment #17)
> Why are you switching away from using initializer list syntax for m_data?
> (This comment repeated for all GraphicsContext implementations.)

Ehm. Disregard this, I am an idiot. :)

[Renata Hodovan]

Created attachment 75799 [details]
Patch

[Andreas Kling]

Comment on attachment 75799 [details]
Patch

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

> WebCore/ChangeLog:9
> +        and  m_common member became unnecessary. They are removed.

Double space after 'and'.
s/m_common member/m_common/

> WebCore/ChangeLog:10
> +        Add two virtual methods to GraphicsContext: platformInit() and platformDestroy(), which

Not virtual anymore.

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

These methods should be private.

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

Should be "const GraphicsContext& state() const" since the method doesn't modify any GraphicsContext members.

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:75
> +    GraphicsContextState state = context->state();

Avoid the unnecessary copy:
const GraphicsContextState& state = context->state();

> WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:96
> +    GraphicsContextState state = context->state();

Ditto.

[Renata Hodovan]

Created attachment 75806 [details]
Patch

[Andreas Kling]

Comment on attachment 75806 [details]
Patch

Excellent, r=me

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 75806 [details]:

http/tests/misc/script-async.html
java/lc3/JSObject/ToObject-001.html

Please file bugs against the tests.  These tests were authored by [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 75806 [details]
Patch

Clearing flags on attachment: 75806

Committed r73492: <http://trac.webkit.org/changeset/73492>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yuta Kitamura]

Hi,

Your change caused a LOT of pixel test failures on Chromium's canary bot. Sounds like Linux bot is producing ~350 test failures. Windows and Mac bots are still compiling, and I guess they are likely to fail as well.

As far as I looked at the test results, <input> controls are not rendered correctly (border of input text box is missing, radio button is not rendered at all, and so forth). Do you have any ideas about this breakage? I'd like to hear from you as soon as possible because these failures seems pretty severe.

[Yuta Kitamura]

I confirmed that Mac WebKit (non-Chromium) also causes a lot of pixel test failures as well.

I'm going to roll out this change for this reason.

[Yuta Kitamura]

Reverted r73492 for reason:

Caused a lot of pixel test failures and broke Windows build

Committed r73496: <http://trac.webkit.org/changeset/73496>

[Andreas Kling]

@Reni: The problem here was most likely the missing initialization of GraphicsContext::m_updatingControlTints

[Renata Hodovan]

Created attachment 76191 [details]
Patch

[WebKit Review Bot]

Attachment 76191 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6991032

[Early Warning System Bot]

Attachment 76191 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6975024

[Renata Hodovan]

Created attachment 76193 [details]
Patch

[Build Bot]

Attachment 76191 [details] did not build on win:
Build output: http://queues.webkit.org/results/6906041

[Andreas Kling]

This was landed in <http://trac.webkit.org/changeset/73728>, closing..