Bug 55150 – [Cairo] Better separate the concerns of GraphicsContextCairo

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

Since GraphicsContextPlatformPrivate is an increasingly heavyweight class we should fix the member naming and hide it's members behind getters and setters.

[Martin Robinson]

I renamed this bug because I think that instead of just fixing the member naming of the private data of the Cairo context, this bug should track cleaning up the private data in general. I would like to see us split out the private data into two sections, the private data which would be totally internal to the GraphicsContext and into a new PlatformContextCairo (like in the Skia) port which exposes things to external classes such as Font and Image.

[Martin Robinson]

Created attachment 84800 [details]
Patch introducing PlatformContextCairo

[Collabora GTK+ EWS bot]

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

[Martin Robinson]

Created attachment 84821 [details]
Patch with missing files

[Carlos Garcia Campos]

Comment on attachment 84821 [details]
Patch with missing files

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

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:205
> +    m_data = new GraphicsContextPlatformPrivate(platformContext);;

There's an additional trailing ; here

[Brent Fulgham]

Comment on attachment 84821 [details]
Patch with missing files

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

Looks fine to me.  My only minor nit is that the repeated stanzas of code required to go from cairo_t* to GraphicsContext seem unattractive.

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:290
> +        GraphicsContext gc(&platformContext);

Would a GraphicsContext constructor from cairo_t make sense here?  Seems inconvenient to have to create these temporaries if this is done in a lot of places.

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:316
> +    GraphicsContext gc(&platformContext);

Ditto above...

> Source/WebCore/platform/graphics/win/ImageCairoWin.cpp:83
> +    GraphicsContext gc(&platformContext);

Again, declaring a stack temporary just to convert to GraphicsContext.

[Martin Robinson]

(In reply to comment #6)
> (From update of attachment 84821 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84821&action=review

Thanks for the comments.

> Looks fine to me.  My only minor nit is that the repeated stanzas of code required to go from cairo_t* to GraphicsContext seem unattractive.

Many of them will go away over time as I move more code into PlatformContextCairo. 

> > Source/WebCore/platform/graphics/cairo/PathCairo.cpp:290
> > +        GraphicsContext gc(&platformContext);
> 
> Would a GraphicsContext constructor from cairo_t make sense here?  Seems inconvenient to have to create these temporaries if this is done in a lot of places.
> 
> > Source/WebCore/platform/graphics/cairo/PathCairo.cpp:316
> > +    GraphicsContext gc(&platformContext);
> 
> Ditto above...
> 
> > Source/WebCore/platform/graphics/win/ImageCairoWin.cpp:83
> > +    GraphicsContext gc(&platformContext);
> 
> Again, declaring a stack temporary just to convert to GraphicsContext.

There are a few options here. We can either allocate the PlatformContext on the stack like we do, or make the PlatformContext reference counted. I believe I attempted the reference counted approach at some point, but found it to be messier.

[Martin Robinson]

(In reply to comment #7)

> There are a few options here. We can either allocate the PlatformContext on the stack like we do, or make the PlatformContext reference counted. I believe I attempted the reference counted approach at some point, but found it to be messier.

I suppose the other option would be to make the GraphicsContextPrivateWinCairo class shared between both ports and have a specialized constructor just for Cairo. I was kind of hoping to remove all Cairo-realted ifdefs from the code eventually. I'll give this a shot though. It might be fairly clean.

[Martin Robinson]

Created attachment 85096 [details]
Patch with Carlos' and Brett's suggestions

[Dirk Schulze]

Comment on attachment 85096 [details]
Patch with Carlos' and Brett's suggestions

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

LGTM. r=me. Just a minor change before landing:

> Source/WebCore/platform/graphics/win/GraphicsContextCairoWin.cpp:134
> +    cairo_scale(cr, 1.0, -1.0);

can you change it to webkit style and remove the dot please?

[Martin Robinson]

Committed r82496: <http://trac.webkit.org/changeset/82496>

[WebKit Review Bot]

http://trac.webkit.org/changeset/82496 might have broken EFL Linux Release (Build)

[Philippe Normand]

Reopening and rolling out. This patch and the 8 (so far) consequent "build fixes" heavily broke the 3 GTK buildslaves. It seems there's a memory leak somewhere, further investigation is needed but for now we need green bots.

Commits:

d39efe2f959a849e808b08726c0e95d4353b6645 r82551
cf8651a8a2919c1dd0aeef863bc5687d0f692122 r82526
3c040019368151ca73ae4782e678da6b204682c4 r82549
cfe6d231f09039ce5f6174ddfbebcc4395427f72 r82548
1f799e21426193c9952386fc0da51a156bd5918e r82544
112fb6e0edeac74b2a8a753352db7273ed7f0ad6 r82541
9a936c79aa9eb5349c742f1bc38e4a5a9237c94d r82513
1ec45f8df5bdda46b2d003d8d428307d64d49092 r82504
03d82ab57a2ba5955e52792bf9daf02f57f46e63 r82496

The affected ports are GTK, WinCairo, EFL and WebKit2GTK.

http://trac.webkit.org/changeset/82553

[Martin Robinson]

Created attachment 88107 [details]
Patch fixing issues with previous version

[Martin Robinson]

(In reply to comment #14)
> Created an attachment (id=88107) [details]
> Patch fixing issues with previous version

I have uploaded a new version of this patch which:

1. Fixes the build on WinCairo and EFL.
2. Properly initializes m_updatingControlTints when constructing a GraphicsContext with a cairo_t.
3. Makes the PlatformContextCairo destructor virtual, fixing a memory leak.

I've tested this patch on GTK+ and WinCairo.

[Eric Seidel (no email)]

Comment on attachment 88107 [details]
Patch fixing issues with previous version

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

Looks sane to me.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1208
> +    cairo_t* cr = platformContext()->cr();

seems you might want a cr() method on GraphicsContext itself give how often you call this.

[Martin Robinson]

(In reply to comment #16)
> seems you might want a cr() method on GraphicsContext itself give how often you call this.

Thanks for the review! I will do this in a followup patch in the interest of keeping this one smaller.

[Martin Robinson]

Committed r82962: <http://trac.webkit.org/changeset/82962>