Bug 23027 – Exclude WebKitGraphics from Redistributable WebKit.dll API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brent Fulgham]

This patch updates the WebKitGraphics.h/.cpp class to use the more portable PlatformGraphicsContext types so that more back-ends could use the Windows API.

[Brent Fulgham]

Created attachment 26307 [details]
Change CG types to PlatformGraphicsContext

[Darin Adler]

Comment on attachment 26307 [details]
Change CG types to PlatformGraphicsContext

I think there's a misunderstanding here. The whole point of this API is that it's something you can use without using WebCore-specific types. If you can use those types, then you can use the WebCore functions directly.

Also, this API is used by Safari, so changing it will break compatibility of nightly builds with the existing Safari.

I think the whole concept here is flawed. If we change this API we need a way to keep existing clients compatible. And the use you're putting this API to could probably be satisfied another way -- if the client has access to WebCore's platform directory and those headers it can use it directly and doesn't need this WebKitGraphics.h wrapper.

[Brent Fulgham]

(In reply to comment #2)
> Also, this API is used by Safari, so changing it will break compatibility of
> nightly builds with the existing Safari.

This shouldn't change anything, or at least it builds and compiles using CG or Cairo.

The only thing that is changing here is that rather than directly using the CGContext type directly, we are using a typedef that is either CGContext (for CG based WebKit.dll), or cairo_t (for Cairo-backed WebKit.dll).

There is no change in functionality, data types, etc.

I included WebCore/GraphicsContext.h because it provides a convenient set of typedefs for these types, conditionalized by the platform in use.

An alternative patch would just #if/def in the header file and typedef to CGContext or cairo_t.

> 
> I think the whole concept here is flawed. If we change this API we need a way
> to keep existing clients compatible. And the use you're putting this API to
> could probably be satisfied another way -- if the client has access to
> WebCore's platform directory and those headers it can use it directly and
> doesn't need this WebKitGraphics.h wrapper.
> 

I think I was not clear in my description.  This change does not make any changes to the functionality or data types used in this file.  It merely allows me to compile using Cairo types if desired so that I can build a WebKit.dll that does not rely on CG.

[Darin Adler]

(In reply to comment #3)
> This shouldn't change anything, or at least it builds and compiles using CG or
> Cairo.
> 
> The only thing that is changing here is that rather than directly using the
> CGContext type directly, we are using a typedef that is either CGContext (for
> CG based WebKit.dll), or cairo_t (for Cairo-backed WebKit.dll).
> 
> There is no change in functionality, data types, etc.
> 
> I included WebCore/GraphicsContext.h because it provides a convenient set of
> typedefs for these types, conditionalized by the platform in use.
> 
> An alternative patch would just #if/def in the header file and typedef to
> CGContext or cairo_t.

Now I understand.

This still won't work as-is. The platform conditionals are not visible outside the project, so we can't use #ifdef in public headers used by clients. The conditionals control how WebKit is built, but don't need to be set by all programs using WebKit.

Instead, we need to either completely avoid these types in public header files, use two copies of the relevant source files, or generate the relevant files.

For this specific example, I suggest you consider treating these headers as Safari-only features and not bother to try to make non-CoreGraphics versions of them. There's no general requirement to have these WebKitGraphics.h functions for general purpose WebKit clients.

But if you do want to create non-CoreGraphics versions it's fine -- we just need to either implement the "two copies of headers" design or the "generate headers" design.

[Brent Fulgham]

(In reply to comment #4)
> For this specific example, I suggest you consider treating these headers as
> Safari-only features and not bother to try to make non-CoreGraphics versions of
> them. There's no general requirement to have these WebKitGraphics.h functions
> for general purpose WebKit clients.

I'm not sure I understand what you mean here -- these routines most definitely are used by clients like WinLauncher.

To not follow the existing WebKit.dll API implies the creation of a whole parallel API structure for clients that happen to be using the non-CG version of WebKit.dll.  I desperately want to avoid this!

Now, if you are suggesting that header files that make use of CoreGraphics types be duplicated I could probably live with that, but I don't really want to have to duplicate the various implementation files that make use of the graphic primitives declared in these headers.

I'm not sure which you are suggesting:  (a) the 'duplicate everything' approach, or (b) 'duplicate just some headers'.

[Brent Fulgham]

Created attachment 26341 [details]
Patch to remove WebKitGraphics.cpp from the Cairo build.

Revised patch for new approach:  WebKitGraphics is not used by the Cairo build, and should not be built or linked into the DLL.

[Oliver Hunt]

Comment on attachment 26341 [details]
Patch to remove WebKitGraphics.cpp from the Cairo build.

r=me, watch the other windows builds however

[Brent Fulgham]

Created attachment 26436 [details]
Without \r line endings

[Brent Fulgham]

Created attachment 26440 [details]
Revised patch against ToT

Previously approved patch, updated for today's ToT to ease landing.

[Brent Fulgham]

Created attachment 26441 [details]
Revised patch against ToT, no DOS-style line endings.

[Matt Lilek]

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