Bug 108978 – Make TestWebKitAPI work for iOS

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[David Farler]

Make TestWebKitAPI work for iOS:

- Don't build WK2 tests
- Guard importing Cocoa and WK2 C API headers
- Export WebCore::KURL::hasPath() with the rest of the getters (needed to build KURL tests on iOS)

[David Farler]

Created attachment 186731 [details]
Patch

[David Farler]

Created attachment 186733 [details]
Patch

[Build Bot]

Comment on attachment 186733 [details]
Patch

Attachment 186733 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16387057

[David Farler]

Waiting for https://bugs.webkit.org/show_bug.cgi?id=107863 so I can add this to the modules list when building on an iOS SDK.

[David Farler]

Created attachment 187214 [details]
Patch

[David Farler]

Created attachment 187215 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 187215 [details]
Patch

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.

[Benjamin Poulain]

Comment on attachment 187215 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187215&action=review

> Tools/TestWebKitAPI/Configurations/Base.xcconfig:25
> +#include "FeatureDefines.xcconfig"
> +

Oh no, one more of those :(

> Tools/TestWebKitAPI/ios/main.mm:37
> +    NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
> +
> +    bool passed = TestWebKitAPI::TestsController::shared().run(argc, argv);
> +
> +    [pool drain];
> +
> +    return passed ? EXIT_SUCCESS : EXIT_FAILURE;

Can we use ARC here?

[Benjamin Poulain]

> > 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?).

This export is in the block USE(WTFURL). That is correct, it just need to be moved to be sorted.

[David Farler]

Created attachment 187365 [details]
Patch

Patch updated with the above suggestions. I didn't turn on ARC since there were complications with WTF but I can follow up later on that. I also removed the need for bringing in a copy of the FeatureDefines.xcconfig.

[David Kilzer (:ddkilzer)]

Comment on attachment 187365 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187365&action=review

This looks good, but I don't understand how a method defined in a header can be exported.  Have you tried compiling with USE(WTFURL) enabled?

> Source/WebCore/WebCore.exp.in:2709
> +__ZNK7WebCore4KURL7hasPathEv

Again, how can we export a method that's defined in a header file?  I don't understand why this works.

Is USE(WTFURL) turned on for any port yet (Benjamin)?

[Benjamin Poulain]

> > Source/WebCore/WebCore.exp.in:2709
> > +__ZNK7WebCore4KURL7hasPathEv
> 
> Again, how can we export a method that's defined in a header file?  I don't understand why this works.

The inline version of KURL::hasPath() is defined in a section that is #ifdef-ed out for WTFURL and GoogleURL.

Only the old KURL defines its field as attributes directly. Both WTFURL and GoogleURL have a private object.

> Is USE(WTFURL) turned on for any port yet (Benjamin)?

It works on both AppleWebKit and GTKWebKit, but is not enabled by default anywhere.

[David Farler]

(In reply to comment #11)
> (From update of attachment 187365 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187365&action=review
> 
> This looks good, but I don't understand how a method defined in a header can be exported.  Have you tried compiling with USE(WTFURL) enabled?
> 
> > Source/WebCore/WebCore.exp.in:2709
> > +__ZNK7WebCore4KURL7hasPathEv
> 
> Again, how can we export a method that's defined in a header file?  I don't understand why this works.
> 
> Is USE(WTFURL) turned on for any port yet (Benjamin)?

I think it’s because:

WebCore/platform/KURL.cpp:
73 #if !USE(GOOGLEURL) && !USE(WTFURL)
...
566 bool KURL::hasPath() const
...
663 #endif(In reply to comment #11)
> (From update of attachment 187365 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187365&action=review
> 
> This looks good, but I don't understand how a method defined in a header can be exported.  Have you tried compiling with USE(WTFURL) enabled?
> 
> > Source/WebCore/WebCore.exp.in:2709
> > +__ZNK7WebCore4KURL7hasPathEv
> 
> Again, how can we export a method that's defined in a header file?  I don't understand why this works.
> 
> Is USE(WTFURL) turned on for any port yet (Benjamin)?

WebCore/platform/KURL.h:
335 #if !USE(GOOGLEURL) && !USE(WTFURL)
various is/has methods inlined here

WebCore/platform/KURL.cpp:
73 #if !USE(GOOGLEURL) && !USE(WTFURL)
...
566 bool KURL::hasPath() const

[David Kilzer (:ddkilzer)]

Comment on attachment 187365 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187365&action=review

r=me!

>>>> Source/WebCore/WebCore.exp.in:2709
>>>> +__ZNK7WebCore4KURL7hasPathEv
>>> 
>>> Again, how can we export a method that's defined in a header file?  I don't understand why this works.
>>> 
>>> Is USE(WTFURL) turned on for any port yet (Benjamin)?
>> 
>> I think it’s because:
>> 
>> WebCore/platform/KURL.cpp:
>> 73 #if !USE(GOOGLEURL) && !USE(WTFURL)
>> ...
>> 566 bool KURL::hasPath() const
>> ...
>> 663 #endif(In reply to comment #11)
> 
> WebCore/platform/KURL.h:
> 335 #if !USE(GOOGLEURL) && !USE(WTFURL)
> various is/has methods inlined here
> 
> WebCore/platform/KURL.cpp:
> 73 #if !USE(GOOGLEURL) && !USE(WTFURL)
> ...
> 566 bool KURL::hasPath() const

I understand now.  I had difficulty comparing the logic between KURL.h and WebCore.exp.in (trying to compare positive logic with negative logic).

[WebKit Review Bot]

Comment on attachment 187365 [details]
Patch

Clearing flags on attachment: 187365

Committed r142381: <http://trac.webkit.org/changeset/142381>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

This seems to have broken the Lion build:
http://build.webkit.org/builders/Apple%20Lion%20Release%20%28Build%29

[David Farler]

(In reply to comment #17)
> This seems to have broken the Lion build:
> http://build.webkit.org/builders/Apple%20Lion%20Release%20%28Build%29

Yes, it looks like Lion doesn’t have a copy of some frameworks under ApplicationServices etc. at /S/L/F. I’ll bring up a patch to add back the search paths.

[David Farler]

Created attachment 187466 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 187466 [details]
Patch

r=me

[Joseph Pecoraro]

Comment on attachment 187466 [details]
Patch

I'll land this, since it is a build fix. Better to get it in early.

[Joseph Pecoraro]

Follow up build fix landed in r142389:
<http://trac.webkit.org/changeset/142389>