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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=108978

Attachment 187215: Patch
https://bugs.webkit.org/attachment.cgi?id=187215&action=review

------- 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
WebCore.exp.in.

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:

GCC_PREPROCESSOR_DEFINITIONS = $(DEBUG_DEFINES)
WEBKIT_VERSION_MIN_REQUIRED=WEBKIT_VERSION_LATEST GTEST_HAS_TR1_TUPLE=0
GTEST_HAS_RTTI=0 $(FEATURE_DEFINES_$(PLATFORM_NAME));
FEATURE_DEFINES_macosx = ENABLE_DASHBOARD_SUPPORT;

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
> +EXCLUDED_SOURCE_FILE_NAMES = $(EXCLUDED_SOURCE_FILE_NAMES_$(PLATFORM_NAME))
$(EXCLUDED_SOURCE_FILE_NAMES_$(CONFIGURATION)_$(PLATFORM_NAME));

Why does this need
$(EXCLUDED_SOURCE_FILE_NAMES_$(CONFIGURATION)_$(PLATFORM_NAME))?  Seems like
this would do:

EXCLUDED_SOURCE_FILE_NAMES = $(EXCLUDED_SOURCE_FILE_NAMES_$(PLATFORM_NAME));

> Tools/TestWebKitAPI/Configurations/TestWebKitAPI.xcconfig:27
> +OTHER_LDFLAGS = $(OTHER_LDFLAGS_$(PLATFORM_NAME)) -lgtest -framework
JavaScriptCore -framework WebKit;

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

OTHER_LDFLAGS = -lgtest -framework JavaScriptCore -framework WebKit
$(OTHER_LDFLAGS_$(PLATFORM_NAME));

> Tools/TestWebKitAPI/config.h:46
>  #ifdef __OBJC__
> +#if !PLATFORM(IOS)
>  #import <Cocoa/Cocoa.h>
>  #endif
> +#endif

This should probably be:

#ifdef __OBJC__
#if PLATFORM(IOS)
#import <Foundation/Foundation.h>
#else
#import <Cocoa/Cocoa.h>
#endif
#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.