Bug 16095 – Move GraphicsContextPrivate to its own header file.

• Component: Platform
• Status: RESOLVED FIXED

[Marc-Antoine Ruel]

By moving class GraphicsContextPrivate to its own header file, in conjunction with GraphicsContext::createGraphicsContextPrivate and GraphicsContext::destroyGraphicsContextPrivate use, it is easier to subclass GraphicsContextPrivate and lesser the necessity of GraphicsContext::m_data.

Note: I didn't update the various platform projects (xcode, vcproj, etc) They need to add WebCore/platform/graphics/GraphicsContextPrivate.h.

[Marc-Antoine Ruel]

Created attachment 17449 [details]
patch that creates WebCore/platform/graphics/GraphicsContextPrivate.h

[Maciej Stachowiak]

Comment on attachment 17449 [details]
patch that creates WebCore/platform/graphics/GraphicsContextPrivate.h

r=me

[Alp Toker]

Created attachment 18192 [details]
Updated patch

This is an updated version of the patch with added header guards, corrected includes and fixed whitespace.

The added feature seems unusable since, even if the classes are subclassed, there is still static code instantiating the base class in GraphicsContext. In light of this, I think this patch is incomplete or wrong.

[Darin Adler]

(In reply to comment #3)
> The added feature seems unusable since, even if the classes are subclassed,
> there is still static code instantiating the base class in GraphicsContext. In
> light of this, I think this patch is incomplete or wrong.

Sure, this patch just moves the class to a separate header file -- it doesn't do what's needed to create a new feature. So I'm not sure it's all that valuable by itself.

[Marc-Antoine Ruel]

I wasn't clear enough, my bad. I'll try to explain better this time.

There is 2 goals:
- Add access to the current GraphicsContextState stack to GraphicsContextPlatformPrivate, by having GraphicsContextPlatformPrivate inherits from GraphicsContextPrivate.
- While at it, remove a memory allocation, clean things up.

There was a confusion about subclassing. I never meant to subclass GraphicsContext but to add the possibility to subclass GraphicsContextPrivate. This subclassing is not about adding anything virtual but really adding all the GraphicsContext private stuff in one place instead of 2 differents objects (GraphicsContextPrivate and GraphicsContextPlatformPrivate). Hence, GraphicsContextPlatformPrivate could really just be subclass of GraphicsContextPrivate. This is not an obligation with this patch though.

This also removes the need of m_data. Since the GraphicsContext constructor could instantiate a subclass of GraphicsContextPrivate and set its pointer in m_common, the need of a second memory allocation (m_data) disapear.

Frankly, I'd rather not modify the way the ports currently use m_data _in this patch_ and instead leave the m_data variable there. At worst it's a sizeof(void*) bytes lost.

This indeeds deprecate m_data but I didn't want to play in every port to modify their use of m_data. This patch is the first step. I can't build all the supported platforms so I'm not fond of modifying platforms I can't test. (!) All of Cairo, GC, Qt, Wx and Win uses m_data. Luckily they all use a fairly simple GraphicsContextPlatformPrivate class which would be easy to convert to a GraphicsContextPrivate subclass.

Do you prefer me to add documentation to m_data about its deprecation in this patch?

I hope it clears things up.

[Eric Seidel (no email)]

Comment on attachment 18192 [details]
Updated patch

This looks good to me (I'm always a fan of breaking classes out into their own files).  I'll leave this for darin/alp to comment on before landing.

[Eric Seidel (no email)]

CCing darin and alp in case they have comments before this lands.

[Alp Toker]

(In reply to comment #7)
> CCing darin and alp in case they have comments before this lands.
> 

Fine by me as long as a comment is added alongside m_data noting that it's deprecated. We shouldn't keep two ways of keeping custom data in SVN for too long and it should be made clear that m_data is on its way out.

[Darin Adler]

Committed revision 30933.