[webkit-reviews] review denied: [Bug 108978] Make TestWebKitAPI work for iOS : [Attachment 187215] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Feb 7 22:31:27 PST 2013

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has denied David Farler
<dfarler at apple.com>'s request for review:
Bug 108978: Make TestWebKitAPI work for iOS

Attachment 187215: Patch

------- Additional Comments from David Kilzer (:ddkilzer) <ddkilzer at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=187215&action=review

r- to address issues below.

> Source/WebCore/WebCore.exp.in:2714
>  __ZNK7WebCore4KURL7isValidEv
> +__ZNK7WebCore4KURL7hasPathEv

These should be sorted alphabetically.

Actually how is there an export symbol for KURL::hasPath() if it becomes an
inline method in KURL.h?  I think this symbol can just be removed from

This probably doesn't fail to build because USE(WTFURL) isn't enabled (yet?).

> Tools/ChangeLog:12
> +	   * TestWebKitAPI/Configurations/Base.xcconfig:
> +	   - Include FeatureDefines
> +	   - Remove VALID_ARCHS
> +	   - Excluded source files per platform

Might be nice to explain why FRAMEWORK_SEARCH_PATHS was removed here.

> Tools/ChangeLog:13
> +	   * TestWebKitAPI/Configurations/FeatureDefines.xcconfig: Added.

Was FeatureDefines.xcconfig added just to get ENABLE_DASHBOARD_SUPPORT?  If so,
I would probably just make GCC_PREPROCESSOR_DEFINITIONS include some kind of
platform feature defines like:


While it's technically correct to use FeatureDefines.xcconfig, the maintenance
burden of adding yet another copy (which will get out of sync faster than the
other copies, most likely, because it will be forgotten first) doesn't outweigh
the simplicity of adding a bit of code for the macosx build.

If we start using a lot more ENABLE_FOO feature macros in TestWebKitAPI, then I
agree we should take the maintenance hit of copying FeatureDefines.xcconfig
into TestWebKitAPI.

> Tools/TestWebKitAPI/Configurations/Base.xcconfig:68

Why does this need
this would do:


> Tools/TestWebKitAPI/Configurations/TestWebKitAPI.xcconfig:27
JavaScriptCore -framework WebKit;

Typically the $(OTHER_LDFLAGS_$(PLATFORM_NAME)) goes at the end of the list:

OTHER_LDFLAGS = -lgtest -framework JavaScriptCore -framework WebKit

> Tools/TestWebKitAPI/config.h:46
>  #ifdef __OBJC__
>  #import <Cocoa/Cocoa.h>
>  #endif
> +#endif

This should probably be:

#ifdef __OBJC__
#import <Foundation/Foundation.h>
#import <Cocoa/Cocoa.h>
#endif // defined(__OBJC__)

> Tools/TestWebKitAPI/ios/main.mm:29
> +int main(int argc, char** argv)

Should we call this mainIOS.mm to differentiate it from mac/main.mm (and rename
that to mac/mainMac.mm)?

It would make the EXCLUDED_SOURCE_FILE_NAMES rules a bit cleaner.

More information about the webkit-reviews mailing list