Bug 49914 - Merge GraphicsContextPrivate into GraphicsContext
Summary: Merge GraphicsContextPrivate into GraphicsContext
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Renata Hodovan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-22 09:10 PST by Andreas Kling
Modified: 2010-12-10 08:03 PST (History)
10 users (show)

See Also:


Attachments
Patch (95.26 KB, patch)
2010-11-30 09:11 PST, Renata Hodovan
kling: review-
Details | Formatted Diff | Diff
Patch (52.52 KB, patch)
2010-12-02 06:29 PST, Renata Hodovan
no flags Details | Formatted Diff | Diff
Patch (53.69 KB, patch)
2010-12-06 12:32 PST, Renata Hodovan
no flags Details | Formatted Diff | Diff
Patch (53.77 KB, patch)
2010-12-06 14:26 PST, Renata Hodovan
kling: review-
Details | Formatted Diff | Diff
Patch (55.62 KB, patch)
2010-12-07 04:51 PST, Renata Hodovan
kling: review-
Details | Formatted Diff | Diff
Patch (55.73 KB, patch)
2010-12-07 05:59 PST, Renata Hodovan
no flags Details | Formatted Diff | Diff
Patch (55.17 KB, patch)
2010-12-10 07:01 PST, Renata Hodovan
no flags Details | Formatted Diff | Diff
Patch (55.16 KB, patch)
2010-12-10 07:24 PST, Renata Hodovan
kling: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2010-11-22 09:10:33 PST
From GraphicsContext.h:

GraphicsContextPrivate* m_common;
GraphicsContextPlatformPrivate* m_data; // Deprecated; m_commmon can just be downcasted. To be removed.
Comment 1 Renata Hodovan 2010-11-30 09:11:25 PST
Created attachment 75155 [details]
Patch
Comment 2 Andreas Kling 2010-11-30 09:22:12 PST
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)
Comment 3 Eric Seidel (no email) 2010-11-30 09:44:50 PST
Attachment 75155 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6461021
Comment 4 WebKit Review Bot 2010-11-30 09:48:16 PST
Attachment 75155 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/6328116
Comment 5 Balazs Kelemen 2010-11-30 09:52:49 PST
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.
Comment 6 Andreas Kling 2010-11-30 10:06:32 PST
(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. :)
Comment 7 Andreas Kling 2010-11-30 13:17:19 PST
Comment on attachment 75155 [details]
Patch

Resetting r- as the patch has numerous issues. (Guess the re-r? was accidental.)
Comment 8 Balazs Kelemen 2010-11-30 15:09:10 PST
(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.
Comment 9 Andreas Kling 2010-12-01 01:29:12 PST
(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. :)
Comment 10 Renata Hodovan 2010-12-02 06:29:00 PST
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.
Comment 11 Balazs Kelemen 2010-12-02 06:46:04 PST
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! :)
Comment 12 Andreas Kling 2010-12-03 06:12:12 PST
Comment on attachment 75370 [details]
Patch

Removing r? since Reni is working on a new version of this patch.
Comment 13 Renata Hodovan 2010-12-06 12:32:37 PST
Created attachment 75729 [details]
Patch
Comment 14 Renata Hodovan 2010-12-06 14:26:58 PST
Created attachment 75737 [details]
Patch
Comment 15 Build Bot 2010-12-06 16:18:37 PST
Attachment 75737 [details] did not build on win:
Build output: http://queues.webkit.org/results/6851058
Comment 16 Andreas Kling 2010-12-06 21:04:04 PST
(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'
Comment 17 Andreas Kling 2010-12-06 21:17:57 PST
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.)
Comment 18 Andreas Kling 2010-12-07 03:14:33 PST
(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. :)
Comment 19 Renata Hodovan 2010-12-07 04:51:27 PST
Created attachment 75799 [details]
Patch
Comment 20 Andreas Kling 2010-12-07 05:31:04 PST
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.
Comment 21 Renata Hodovan 2010-12-07 05:59:56 PST
Created attachment 75806 [details]
Patch
Comment 22 Andreas Kling 2010-12-07 06:02:37 PST
Comment on attachment 75806 [details]
Patch

Excellent, r=me
Comment 23 WebKit Commit Bot 2010-12-07 23:34:59 PST
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 ap@webkit.org and tonyg@chromium.org.  The commit-queue is continuing to process your patch.
Comment 24 WebKit Commit Bot 2010-12-08 00:05:36 PST
Comment on attachment 75806 [details]
Patch

Clearing flags on attachment: 75806

Committed r73492: <http://trac.webkit.org/changeset/73492>
Comment 25 WebKit Commit Bot 2010-12-08 00:05:45 PST
All reviewed patches have been landed.  Closing bug.
Comment 26 Yuta Kitamura 2010-12-08 01:07:34 PST
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.
Comment 27 Yuta Kitamura 2010-12-08 01:26:55 PST
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.
Comment 28 Yuta Kitamura 2010-12-08 01:35:22 PST
Reverted r73492 for reason:

Caused a lot of pixel test failures and broke Windows build

Committed r73496: <http://trac.webkit.org/changeset/73496>
Comment 29 Andreas Kling 2010-12-09 02:16:12 PST
@Reni: The problem here was most likely the missing initialization of GraphicsContext::m_updatingControlTints
Comment 30 Renata Hodovan 2010-12-10 07:01:46 PST
Created attachment 76191 [details]
Patch
Comment 31 WebKit Review Bot 2010-12-10 07:11:37 PST
Attachment 76191 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6991032
Comment 32 Early Warning System Bot 2010-12-10 07:17:55 PST
Attachment 76191 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6975024
Comment 33 Renata Hodovan 2010-12-10 07:24:30 PST
Created attachment 76193 [details]
Patch
Comment 34 Build Bot 2010-12-10 07:47:09 PST
Attachment 76191 [details] did not build on win:
Build output: http://queues.webkit.org/results/6906041
Comment 35 Andreas Kling 2010-12-10 08:03:31 PST
This was landed in <http://trac.webkit.org/changeset/73728>, closing..