Bug 40058

Summary: testapi.c depends on the Core Foundation.
Product: WebKit Reporter: Jędrzej Nowacki <jedrzej.nowacki>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: aroben, cmarcelo, commit-queue, eric, jedrzej.nowacki, jesus, kent.hansen, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 42229    
Bug Blocks: 31863    
Attachments:
Description Flags
Separate CF parts of testapi, compile support for Qt environment
oliver: review+
Qt version of the assert function that depended on CF
none
Tests for JSStringCreateWithCharacters
none
Separate CF parts of testapi, compile support for Qt environment, take 2
none
Implement the assertEqualsAsCharactersPtr() for Qt
none
Separate CF parts of testapi, compile support for Qt environment, take 3
none
Separate CF parts of testapi, compile support for Qt environment, take 4
none
Separate CF parts of testapi, compile support for Qt environment, take 5 cfleizach: review-

Description Jędrzej Nowacki 2010-06-02 06:57:33 PDT
JSC C API tests could be compile only on Mac. It means that the API is not tested on other platforms and it could not be easily fixed or developed by people not using Mac.

I strongly believe that the dependency should be removed.
Comment 1 Caio Marcelo de Oliveira Filho 2010-07-01 16:27:23 PDT
Created attachment 60308 [details]
Separate CF parts of testapi, compile support for Qt environment

This bootstraps support for Qt (and possible other platforms that have interest) in compiling the testapi.c. The build system bits were taken from jsc.pro.
Comment 2 Caio Marcelo de Oliveira Filho 2010-07-01 16:31:24 PDT
Created attachment 60309 [details]
Qt version of the assert function that depended on CF

The WithCharacters API functions of JSStringRef.h work with 16-bit unicode characters. To test this representation the testapi.c used CFString and its helper functions. This patch implements a Qt version using QString.

Since it depends on the previous patch, I didn't mark for reviewing yet, but feel free to point out whatever problems you find out or whether there's a better way to do it.
Comment 3 Caio Marcelo de Oliveira Filho 2010-07-01 16:35:34 PDT
Created attachment 60310 [details]
Tests for JSStringCreateWithCharacters

Depends on previous patches.

JSStringCreateWithCharacters() tests depended on CF specific functions. This commit adds tests that depend on a more generic function that both CF and Qt implement. This function creates the JSStringRef using CFString or QString as helpers to get the 16-bit buffer and then feed JSStringCreateWithCharacters(). It's similar in spirit to the assert of the previous patch.

Note: I couldn't test the CF version, so would be glad if someone with CF available could test it.
Comment 4 Caio Marcelo de Oliveira Filho 2010-07-01 16:40:34 PDT
I added the three patches here since they are related to the same issue and depend on each other (2nd depends on 1st and 3rd on 2nd).
Comment 5 WebKit Review Bot 2010-07-02 02:59:57 PDT
Attachment 60308 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3345403
Comment 6 Oliver Hunt 2010-07-02 14:36:20 PDT
Comment on attachment 60308 [details]
Separate CF parts of testapi, compile support for Qt environment

r=me
Comment 7 Caio Marcelo de Oliveira Filho 2010-07-06 08:26:27 PDT
How can I get more details from the win build error at

http://webkit-commit-queue.appspot.com/results/3345403
Comment 8 Caio Marcelo de Oliveira Filho 2010-07-13 12:44:16 PDT
Created attachment 61406 [details]
Separate CF parts of testapi, compile support for Qt environment, take 2

Make the function testJSStringRefCF() static. This was generating a compiler error in the CF code path (I tested now on MacOSX but probably it's the win EWS bot error).
Comment 9 WebKit Commit Bot 2010-07-13 17:00:15 PDT
Comment on attachment 61406 [details]
Separate CF parts of testapi, compile support for Qt environment, take 2

Clearing flags on attachment: 61406

Committed r63262: <http://trac.webkit.org/changeset/63262>
Comment 10 WebKit Commit Bot 2010-07-13 17:00:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Caio Marcelo de Oliveira Filho 2010-07-13 20:42:44 PDT
Created attachment 61462 [details]
Implement the assertEqualsAsCharactersPtr() for Qt

Create a Qt version of this assertion, to complement the existing CF code. This enables more of the testapi when compiling in the Qt platform.

Note: should this and the other patch go to bugs of their own?
Comment 12 Simon Fraser (smfr) 2010-07-13 21:08:22 PDT
r63262 broke windows builds.
Comment 13 Caio Marcelo de Oliveira Filho 2010-07-13 22:26:16 PDT
Created attachment 61470 [details]
Separate CF parts of testapi, compile support for Qt environment, take 3

The original commit was rolled out because it broke Win compilation (bug 42229). In VCProj, config.h wasn't available in the testapi utility compilation. Sorry folks.

The patch is the same as before, but adds the necessary bits for the Win build system (at least what I thought was necessary). Any suggestions on how to do it better is appreciated, since I'm not familiar with VCProj neiter have a win system available.

I'm marking this for review? but not cq? so the bots can check this patch and we can see if win passes. I will also remove the r? from the other patch to avoid confusion.
Comment 14 Caio Marcelo de Oliveira Filho 2010-07-14 05:16:34 PDT
Created attachment 61506 [details]
Separate CF parts of testapi, compile support for Qt environment, take 4

Updating the patch for the win EWS bot. This time I just added the correct include directory in the compilation. Let's see if the bot can compile it this time.
Comment 15 WebKit Review Bot 2010-07-14 06:31:24 PDT
Attachment 61506 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3497297
Comment 16 Adam Roben (:aroben) 2010-07-14 07:20:16 PDT
Comment on attachment 61506 [details]
Separate CF parts of testapi, compile support for Qt environment, take 4

> -		AdditionalIncludeDirectories="&quot;$(ProjectDir)\..\..\API&quot;;&quot;$(WebKitOutputDir)\include\WebCore\ForwardingHeaders&quot;;&quot;$(WebKitOutputDir)\include\JavaScriptCore&quot;;&quot;$(WebKitOutputDir)\include\private\JavaScriptCore&quot;;&quot;$(WebKitOutputDir)\include&quot;;&quot;$(WebKitOutputDir)\include\private&quot;;&quot;$(WebKitLibrariesDir)\include&quot;;&quot;$(WebKitLibrariesDir)\include\private&quot;"
> +		AdditionalIncludeDirectories="&quot;$(ProjectDir)\..\..\API&quot;;../../;&quot;$(WebKitOutputDir)\include\WebCore\ForwardingHeaders&quot;;&quot;$(WebKitOutputDir)\include\JavaScriptCore&quot;;&quot;$(WebKitOutputDir)\include\private\JavaScriptCore&quot;;&quot;$(WebKitOutputDir)\include&quot;;&quot;$(WebKitOutputDir)\include\private&quot;;&quot;$(WebKitLibrariesDir)\include&quot;;&quot;$(WebKitLibrariesDir)\include\private&quot;"

Please use backslashes and make the path relative to ProjectDir: $(ProjectDir)\..\..
Comment 17 Caio Marcelo de Oliveira Filho 2010-07-14 07:41:45 PDT
Created attachment 61517 [details]
Separate CF parts of testapi, compile support for Qt environment, take 5

Following Adam Roben suggestion.
Comment 18 WebKit Review Bot 2010-07-14 09:22:52 PDT
Attachment 61517 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3529111
Comment 19 chris fleizach 2010-09-22 00:22:37 PDT
Comment on attachment 61517 [details]
Separate CF parts of testapi, compile support for Qt environment, take 5

please fix so it builds on win