Since GraphicsContextPlatformPrivate is an increasingly heavyweight class we should fix the member naming and hide it's members behind getters and setters.
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.
Created attachment 84800 [details] Patch introducing PlatformContextCairo
Attachment 84800 [details] did not build on gtk: Build output: http://queues.webkit.org/results/8087736
Created attachment 84821 [details] Patch with missing files
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 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.
(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.
(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.
Created attachment 85096 [details] Patch with Carlos' and Brett's suggestions
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?
Committed r82496: <http://trac.webkit.org/changeset/82496>
http://trac.webkit.org/changeset/82496 might have broken EFL Linux Release (Build)
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
Created attachment 88107 [details] Patch fixing issues with previous version
(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 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.
(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.
Committed r82962: <http://trac.webkit.org/changeset/82962>