[webkit-reviews] review denied: [Bug 23492] Separating the WebKitSystemInterface Calls : [Attachment 26954] Exclude WebKitSystemInterface calls in non-Apple builds

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jan 23 14:00:08 PST 2009

Darin Adler <darin at apple.com> has denied Brent Fulgham <bfulgham at webkit.org>'s
request for review:
Bug 23492: Separating the WebKitSystemInterface Calls

Attachment 26954: Exclude WebKitSystemInterface calls in non-Apple builds

------- Additional Comments from Darin Adler <darin at apple.com>
>  #include <CoreFoundation/CoreFoundation.h>
>  #include <CoreGraphics/CoreGraphics.h>
> +#endif
>  #include <shlobj.h>
>  #include <shfolder.h>
>  #include <tchar.h>
>  #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>
>  #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.

More information about the webkit-reviews mailing list