Bug 108978

Summary: Make TestWebKitAPI work for iOS
Product: WebKit Reporter: David Farler <dfarler>
Component: Tools / TestsAssignee: David Farler <dfarler>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, buildbot, ddkilzer, joepeck, mrowe, rniwa, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: Other   
Bug Depends on: 107863    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch joepeck: review+, joepeck: commit-queue-

Description David Farler 2013-02-05 13:49:00 PST
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)
Comment 1 David Farler 2013-02-05 17:34:22 PST
Created attachment 186731 [details]
Patch
Comment 2 David Farler 2013-02-05 17:47:14 PST
Created attachment 186733 [details]
Patch
Comment 3 Build Bot 2013-02-05 18:31:45 PST
Comment on attachment 186733 [details]
Patch

Attachment 186733 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16387057
Comment 4 David Farler 2013-02-06 13:23:57 PST
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.
Comment 5 David Farler 2013-02-07 18:36:34 PST
Created attachment 187214 [details]
Patch
Comment 6 David Farler 2013-02-07 18:39:50 PST
Created attachment 187215 [details]
Patch
Comment 7 David Kilzer (:ddkilzer) 2013-02-07 22:33:37 PST
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.
Comment 8 Benjamin Poulain 2013-02-07 22:46:33 PST
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?
Comment 9 Benjamin Poulain 2013-02-07 22:47:05 PST
> > 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.
Comment 10 David Farler 2013-02-08 14:29:46 PST
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.
Comment 11 David Kilzer (:ddkilzer) 2013-02-08 21:12:46 PST
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)?
Comment 12 Benjamin Poulain 2013-02-08 21:23:47 PST
> > 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.
Comment 13 David Farler 2013-02-08 21:41:12 PST
(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
Comment 14 David Kilzer (:ddkilzer) 2013-02-09 14:22:40 PST
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).
Comment 15 WebKit Review Bot 2013-02-09 14:54:51 PST
Comment on attachment 187365 [details]
Patch

Clearing flags on attachment: 187365

Committed r142381: <http://trac.webkit.org/changeset/142381>
Comment 16 WebKit Review Bot 2013-02-09 14:54:56 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Simon Fraser (smfr) 2013-02-09 18:10:30 PST
This seems to have broken the Lion build:
http://build.webkit.org/builders/Apple%20Lion%20Release%20%28Build%29
Comment 18 David Farler 2013-02-09 23:18:27 PST
(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.
Comment 19 David Farler 2013-02-09 23:26:29 PST
Created attachment 187466 [details]
Patch
Comment 20 Joseph Pecoraro 2013-02-09 23:30:32 PST
Comment on attachment 187466 [details]
Patch

r=me
Comment 21 Joseph Pecoraro 2013-02-09 23:39:10 PST
Comment on attachment 187466 [details]
Patch

I'll land this, since it is a build fix. Better to get it in early.
Comment 22 Joseph Pecoraro 2013-02-09 23:40:21 PST
Follow up build fix landed in r142389:
<http://trac.webkit.org/changeset/142389>