Bug 55150

Summary: [Cairo] Better separate the concerns of GraphicsContextCairo
Product: WebKit Reporter: Martin Robinson <mrobinson>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, alex, bfulgham, eric, gustavo.noronha, gustavo, krit, pnormand, webkit.review.bot, xan.lopez
Priority: P3 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch introducing PlatformContextCairo
none
Patch with missing files
none
Patch with Carlos' and Brett's suggestions
none
Patch fixing issues with previous version eric: review+

Description Martin Robinson 2011-02-24 08:30:10 PST
Since GraphicsContextPlatformPrivate is an increasingly heavyweight class we should fix the member naming and hide it's members behind getters and setters.
Comment 1 Martin Robinson 2011-03-03 16:47:44 PST
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.
Comment 2 Martin Robinson 2011-03-04 14:42:36 PST
Created attachment 84800 [details]
Patch introducing PlatformContextCairo
Comment 3 Collabora GTK+ EWS bot 2011-03-04 14:57:51 PST
Attachment 84800 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8087736
Comment 4 Martin Robinson 2011-03-04 16:05:40 PST
Created attachment 84821 [details]
Patch with missing files
Comment 5 Carlos Garcia Campos 2011-03-08 00:16:27 PST
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
Comment 6 Brent Fulgham 2011-03-08 12:33:11 PST
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.
Comment 7 Martin Robinson 2011-03-08 13:02:03 PST
(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.
Comment 8 Martin Robinson 2011-03-08 14:14:20 PST
(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.
Comment 9 Martin Robinson 2011-03-08 14:54:28 PST
Created attachment 85096 [details]
Patch with Carlos' and Brett's suggestions
Comment 10 Dirk Schulze 2011-03-30 10:11:16 PDT
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?
Comment 11 Martin Robinson 2011-03-30 13:56:57 PDT
Committed r82496: <http://trac.webkit.org/changeset/82496>
Comment 12 WebKit Review Bot 2011-03-30 14:41:25 PDT
http://trac.webkit.org/changeset/82496 might have broken EFL Linux Release (Build)
Comment 13 Philippe Normand 2011-03-31 02:55:22 PDT
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
Comment 14 Martin Robinson 2011-04-04 13:08:57 PDT
Created attachment 88107 [details]
Patch fixing issues with previous version
Comment 15 Martin Robinson 2011-04-04 13:12:34 PDT
(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.
Comment 16 Eric Seidel (no email) 2011-04-04 13:13:26 PDT
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.
Comment 17 Martin Robinson 2011-04-05 10:45:52 PDT
(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.
Comment 18 Martin Robinson 2011-04-05 12:19:09 PDT
Committed r82962: <http://trac.webkit.org/changeset/82962>