Bug 23492 – Separating the WebKitSystemInterface Calls

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

[Brent Fulgham]

The Safari build of WebKit relies on a few font and network-related functions that ship in a standalone library (WebKitSystemInterface).  Since this library may not be redistributed, provide alternative implementation for external WebKit users.

This patch creates temporary link stubs for these functions.

[Brent Fulgham]

Created attachment 26954 [details]
Exclude WebKitSystemInterface calls in non-Apple builds

[Darin Adler]

Comment on attachment 26954 [details]
Exclude WebKitSystemInterface calls in non-Apple builds

>  #include <CoreFoundation/CoreFoundation.h>
> +#if PLATFORM(CG)
>  #include <CoreGraphics/CoreGraphics.h>
> +#endif
>  #include <shlobj.h>
>  #include <shfolder.h>
>  #include <tchar.h>
> +#if PLATFORM(CG)
>  #include <WebKitSystemInterface/WebKitSystemInterface.h>
> +#endif
>  #include <wtf/HashMap.h>
>  #include <wtf/OwnArrayPtr.h>

Preferred WebKit style is to put all unconditional includes at the top of the file in one section, then put the conditional includes in a separate block, sorted separately.

> +// Link stubs for non-Safari clients
> +#if !PLATFORM(CG)
> +static void wkSetFontSmoothingLevel(int smoothingLevel) { notImplemented(); }
> +static void wkSetFontSmoothingContrast(float contrast) { notImplemented(); }
> +#endif

Why are these stubs needed? These are CG-specific calls. I think we should instead have #if at the call sites. Since there's only one call to each function, it seems straightforward to do it that way instead.

I don't think that non-Core-Graphics versions of this will have calls with this same design, so I don't think we should have link stubs for them.

>  #include <CoreFoundation/CFString.h>
> +#if PLATFORM(CG)
>  #include <WebKitSystemInterface/WebKitSystemInterface.h>
> +#endif
>  #include <wtf/RetainPtr.h>

Same comment here.
>  
> +// Link stubs for non-Safari clients
> +#if !PLATFORM(CG)
> +#include <WebCore/NotImplemented.h>
> +static void wkAddFontsAtPath(CFStringRef) { notImplemented(); }
> +#endif

Same comment here. I don't think you need a stub for this Core Graphics function call. You just need to put #if PLATFORM(CG) around the part of the WebTextRenderer::registerPrivateFont function that creates a CFStringRef and calls wkAddFontsAtPath.

I'm going to say review-, because I'd prefer not to create "stubs" unless there's some real value there.

[Brent Fulgham]

Created attachment 26983 [details]
Remove calls to WebKitSystemInterface functions

Revised to address Darin's comments.

[Darin Adler]

Comment on attachment 26983 [details]
Remove calls to WebKitSystemInterface functions

> +        https://bugs.webkit.org/show_bug.cgi?id=23492
> +        Exclude calls to WebKitSystemInterface functions since they
> +        are not available outside of the Apple build of WebKit.

Since they are not needed if you're not using Core Graphics. The issue here isn't "Apple build of WebKit", it's "not calling CoreGraphics functions when not using CoreGraphics".

> +#if PLATFORM(CG)
>  #include <CoreGraphics/CoreGraphics.h>
> +#include <WebKitSystemInterface/WebKitSystemInterface.h>
> +#endif
>  #include <shlobj.h>
>  #include <shfolder.h>
>  #include <tchar.h>
> -#include <WebKitSystemInterface/WebKitSystemInterface.h>
>  #include <wtf/HashMap.h>
>  #include <wtf/OwnArrayPtr.h>

The PLATFORM(CG) part should go in its own separate paragraph, after the other includes and separate by a blank line. Sorry I wasn't clear on this before.

r=me

[Brent Fulgham]

Landed in r40203