Summary: | Make TestWebKitAPI work for iOS | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Farler <dfarler> | ||||||||||||||
Component: | Tools / Tests | Assignee: | 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
David Farler
2013-02-05 13:49:00 PST
Created attachment 186731 [details]
Patch
Created attachment 186733 [details]
Patch
Comment on attachment 186733 [details] Patch Attachment 186733 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16387057 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. Created attachment 187214 [details]
Patch
Created attachment 187215 [details]
Patch
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 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? > > 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.
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 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)? > > 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. (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 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 on attachment 187365 [details] Patch Clearing flags on attachment: 187365 Committed r142381: <http://trac.webkit.org/changeset/142381> All reviewed patches have been landed. Closing bug. This seems to have broken the Lion build: http://build.webkit.org/builders/Apple%20Lion%20Release%20%28Build%29 (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. Created attachment 187466 [details]
Patch
Comment on attachment 187466 [details]
Patch
r=me
Comment on attachment 187466 [details]
Patch
I'll land this, since it is a build fix. Better to get it in early.
Follow up build fix landed in r142389: <http://trac.webkit.org/changeset/142389> |