Bug 67756 - Let TestWebKitAPI work for chromium
Summary: Let TestWebKitAPI work for chromium
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-07 20:20 PDT by Xianzhu Wang
Modified: 2011-09-15 07:17 PDT (History)
9 users (show)

See Also:


Attachments
patch (12.17 KB, patch)
2011-09-08 00:57 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
use config.h. Mac and Windows to be verified by bots. (39.64 KB, patch)
2011-09-09 22:42 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
use config.h. Mac and Windows to be verified by bots. (42.97 KB, patch)
2011-09-09 22:45 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xianzhu Wang 2011-09-07 20:20:12 PDT
TestWebKitAPI for now only works on mac and windows. I think it would be good to let it work for chromium port.
Comment 1 Xianzhu Wang 2011-09-08 00:57:01 PDT
Created attachment 106707 [details]
patch
Comment 2 Adam Roben (:aroben) 2011-09-08 06:13:42 PDT
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?
Comment 3 Sam Weinig 2011-09-08 16:35:57 PDT
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()?
Comment 4 Xianzhu Wang 2011-09-09 04:03:29 PDT
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.
Comment 5 Xianzhu Wang 2011-09-09 04:11:42 PDT
(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.
Comment 6 Adam Roben (:aroben) 2011-09-09 05:38:43 PDT
(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?
Comment 7 Xianzhu Wang 2011-09-09 05:45:41 PDT
(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.
Comment 8 Adam Roben (:aroben) 2011-09-09 05:47:56 PDT
Perhaps it would be best to follow the "config.h" approach of other WebKit projects (WebCore, WebKit2, etc.).
Comment 9 Xianzhu Wang 2011-09-09 07:39:09 PDT
(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.
Comment 10 Sam Weinig 2011-09-09 10:35:15 PDT
(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.
Comment 11 Xianzhu Wang 2011-09-09 22:42:52 PDT
Created attachment 106964 [details]
use config.h. Mac and Windows to be verified by bots.
Comment 12 Xianzhu Wang 2011-09-09 22:45:10 PDT
Created attachment 106965 [details]
use config.h. Mac and Windows to be verified by bots.
Comment 13 Xianzhu Wang 2011-09-13 19:52:01 PDT
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.
Comment 14 WebKit Review Bot 2011-09-15 07:07:50 PDT
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>
Comment 15 WebKit Review Bot 2011-09-15 07:07:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 David Levin 2011-09-15 07:14:36 PDT
(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 :)
Comment 17 David Levin 2011-09-15 07:17:05 PDT
btw, it is cool that this runs for Chromium now!