Bug 67756 – Let TestWebKitAPI work for chromium

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Xianzhu Wang]

TestWebKitAPI for now only works on mac and windows. I think it would be good to let it work for chromium port.

[Xianzhu Wang]

Created attachment 106707 [details]
patch

[Adam Roben (:aroben)]

Comment on attachment 106707 [details]
patch

I don't understand the TestWebKitAPI[Prefix].h changes. Why can't Chromium use a prefix header like Mac and Windows do?

[Sam Weinig]

Comment on attachment 106707 [details]
patch

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

> Source/WebKit/chromium/WebKit.gyp:2
> -# Copyright (C) 2010 Google Inc. All rights reserved.
> +# Copyright (C) 2011 Google Inc. All rights reserved.

We usually just add the new year when this is updated, so 2010, 2011.

> Tools/Scripts/run-api-tests:69
> +  --chromium            Run the Chromium port on Mac/Win/Linux

Why is this necessary?  Can't you just call isChromium()?

[Xianzhu Wang]

Comment on attachment 106707 [details]
patch

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

>> Source/WebKit/chromium/WebKit.gyp:2
>> +# Copyright (C) 2011 Google Inc. All rights reserved.
> 
> We usually just add the new year when this is updated, so 2010, 2011.

It's a common practice to use only the last year for Google copyrights, otherwise it should have been "2009, 2010".

>> Tools/Scripts/run-api-tests:69
>> +  --chromium            Run the Chromium port on Mac/Win/Linux
> 
> Why is this necessary?  Can't you just call isChromium()?

isChromium() just looks for '--chromium' in the command line.

[Xianzhu Wang]

(In reply to comment #2)
> (From update of attachment 106707 [details])
> I don't understand the TestWebKitAPI[Prefix].h changes. Why can't Chromium use a prefix header like Mac and Windows do?

I haven't found a way to achieve that. I have asked the question in https://lists.webkit.org/pipermail/webkit-dev/2011-September/017892.html.

[Adam Roben (:aroben)]

(In reply to comment #5)
> (In reply to comment #2)
> > (From update of attachment 106707 [details] [details])
> > I don't understand the TestWebKitAPI[Prefix].h changes. Why can't Chromium use a prefix header like Mac and Windows do?
> 
> I haven't found a way to achieve that. I have asked the question in https://lists.webkit.org/pipermail/webkit-dev/2011-September/017892.html.

 % git grep Prefix -- '*.gyp'
Source/JavaScriptCore/gyp/JavaScriptCore.gyp:            'GCC_PREFIX_HEADER': '<(project_dir)/JavaScriptCorePrefix.h',
Source/WebCore/WebCore.gyp/WebCore.gyp:          'GCC_PREFIX_HEADER': '../WebCorePrefix.h',
Source/WebCore/gyp/WebCore.gyp:            'GCC_PREFIX_HEADER': '<(project_dir)/WebCorePrefix.h',
Source/WebCore/gyp/WebCore.gyp:            'GCC_PREFIX_HEADER': '<(project_dir)/WebCorePrefix.h',

Can we do something similar for TestWebKitAPI?

[Xianzhu Wang]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #2)
> > > (From update of attachment 106707 [details] [details] [details])
> > > I don't understand the TestWebKitAPI[Prefix].h changes. Why can't Chromium use a prefix header like Mac and Windows do?
> > 
> > I haven't found a way to achieve that. I have asked the question in https://lists.webkit.org/pipermail/webkit-dev/2011-September/017892.html.
> 
>  % git grep Prefix -- '*.gyp'
> Source/JavaScriptCore/gyp/JavaScriptCore.gyp:            'GCC_PREFIX_HEADER': '<(project_dir)/JavaScriptCorePrefix.h',
> Source/WebCore/WebCore.gyp/WebCore.gyp:          'GCC_PREFIX_HEADER': '../WebCorePrefix.h',
> Source/WebCore/gyp/WebCore.gyp:            'GCC_PREFIX_HEADER': '<(project_dir)/WebCorePrefix.h',
> Source/WebCore/gyp/WebCore.gyp:            'GCC_PREFIX_HEADER': '<(project_dir)/WebCorePrefix.h',
> 
> Can we do something similar for TestWebKitAPI?

Those are all in 'xcode_settings'. I think we don't need to specially treat mac platform of Chromium as we have already included all necessary headers.

[Adam Roben (:aroben)]

Perhaps it would be best to follow the "config.h" approach of other WebKit projects (WebCore, WebKit2, etc.).

[Xianzhu Wang]

(In reply to comment #8)
> Perhaps it would be best to follow the "config.h" approach of other WebKit projects (WebCore, WebKit2, etc.).

Is it OK to just rename TestWebKitAPI.h to config.h in my patch?

Or the following method?
1) rename TestWebKitAPIPrefix.h to config.h 
2) include config.h from each of the source files
3) disable original GCC_PREFIX_HEADER or ForceIncludeFiles

I prefer the former because the change is small and I don't need to verify my change on both Mac and Windows.

[Sam Weinig]

(In reply to comment #9)
> (In reply to comment #8)
> > Perhaps it would be best to follow the "config.h" approach of other WebKit projects (WebCore, WebKit2, etc.).
> 
> Is it OK to just rename TestWebKitAPI.h to config.h in my patch?
> 
> Or the following method?
> 1) rename TestWebKitAPIPrefix.h to config.h 
> 2) include config.h from each of the source files
> 3) disable original GCC_PREFIX_HEADER or ForceIncludeFiles
> 
> I prefer the former because the change is small and I don't need to verify my change on both Mac and Windows.

Please do the latter.

[Xianzhu Wang]

Created attachment 106964 [details]
use config.h. Mac and Windows to be verified by bots.

[Xianzhu Wang]

Created attachment 106965 [details]
use config.h. Mac and Windows to be verified by bots.

[Xianzhu Wang]

As there will be several changes to StringBuilder, I think it'd better if we have this change landed first. Then we can incrementally add unit tests together with the changes to StringBuilder.

[WebKit Review Bot]

Comment on attachment 106965 [details]
use config.h. Mac and Windows to be verified by bots.

Clearing flags on attachment: 106965

Committed r95188: <http://trac.webkit.org/changeset/95188>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[David Levin]

(In reply to comment #4)
> (From update of attachment 106707 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106707&action=review
> 
> >> Source/WebKit/chromium/WebKit.gyp:2
> >> +# Copyright (C) 2011 Google Inc. All rights reserved.
> > 
> > We usually just add the new year when this is updated, so 2010, 2011.
> 
> It's a common practice to use only the last year for Google copyrights, otherwise it should have been "2009, 2010".

Just a note for the future, when in WebKit, please follow WebKit practices :)

[David Levin]

btw, it is cool that this runs for Chromium now!