Bug 16095

Summary: Move GraphicsContextPrivate to its own header file.
Product: WebKit Reporter: Marc-Antoine Ruel <maruel>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: alp, darin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
patch that creates WebCore/platform/graphics/GraphicsContextPrivate.h
mjs: review+
Updated patch eric: review+

Description Marc-Antoine Ruel 2007-11-22 13:14:29 PST
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.
Comment 1 Marc-Antoine Ruel 2007-11-22 13:18:10 PST
Created attachment 17449 [details]
patch that creates WebCore/platform/graphics/GraphicsContextPrivate.h
Comment 2 Maciej Stachowiak 2007-11-22 19:31:59 PST
Comment on attachment 17449 [details]
patch that creates WebCore/platform/graphics/GraphicsContextPrivate.h

r=me
Comment 3 Alp Toker 2007-12-30 07:07:45 PST
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.
Comment 4 Darin Adler 2008-01-13 11:32:20 PST
(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.
Comment 5 Marc-Antoine Ruel 2008-01-16 09:54:18 PST
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.
Comment 6 Eric Seidel (no email) 2008-01-16 09:59:27 PST
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.
Comment 7 Eric Seidel (no email) 2008-01-16 09:59:59 PST
CCing darin and alp in case they have comments before this lands.
Comment 8 Alp Toker 2008-01-21 04:00:32 PST
(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.
Comment 9 Darin Adler 2008-03-10 09:30:18 PDT
Committed revision 30933.