Bug 23492 - Separating the WebKitSystemInterface Calls
Summary: Separating the WebKitSystemInterface Calls
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-01-22 20:18 PST by Brent Fulgham
Modified: 2009-01-23 17:23 PST (History)
0 users

See Also:


Attachments
Exclude WebKitSystemInterface calls in non-Apple builds (2.75 KB, patch)
2009-01-22 20:26 PST, Brent Fulgham
darin: review-
Details | Formatted Diff | Diff
Remove calls to WebKitSystemInterface functions (2.71 KB, patch)
2009-01-23 14:31 PST, Brent Fulgham
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2009-01-22 20:18:58 PST
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.
Comment 1 Brent Fulgham 2009-01-22 20:26:54 PST
Created attachment 26954 [details]
Exclude WebKitSystemInterface calls in non-Apple builds
Comment 2 Darin Adler 2009-01-23 14:00:08 PST
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.
Comment 3 Brent Fulgham 2009-01-23 14:31:55 PST
Created attachment 26983 [details]
Remove calls to WebKitSystemInterface functions

Revised to address Darin's comments.
Comment 4 Darin Adler 2009-01-23 15:58:01 PST
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
Comment 5 Brent Fulgham 2009-01-23 17:23:52 PST
Landed in r40203