67756 – Let TestWebKitAPI work for chromium

RESOLVED FIXED 67756

Let TestWebKitAPI work for chromium

https://bugs.webkit.org/show_bug.cgi?id=67756

Summary Let TestWebKitAPI work for chromium

Xianzhu Wang

Reported 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.

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Xianzhu Wang

Comment 1 2011-09-08 00:57:01 PDT

Created attachment 106707 [details] patch

Adam Roben (:aroben)

Comment 2 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?

Sam Weinig

Comment 3 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()?

Xianzhu Wang

Comment 4 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.

Xianzhu Wang

Comment 5 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.

Adam Roben (:aroben)

Comment 6 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?

Xianzhu Wang

Comment 7 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.

Adam Roben (:aroben)

Comment 8 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.).

Xianzhu Wang

Comment 9 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.

Sam Weinig

Comment 10 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.

Xianzhu Wang

Comment 11 2011-09-09 22:42:52 PDT

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

Xianzhu Wang

Comment 12 2011-09-09 22:45:10 PDT

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

Xianzhu Wang

Comment 13 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.

WebKit Review Bot

Comment 14 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>

WebKit Review Bot

Comment 15 2011-09-15 07:07:55 PDT

All reviewed patches have been landed. Closing bug.

David Levin

Comment 16 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 :)

David Levin

Comment 17 2011-09-15 07:17:05 PDT

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

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component Tools / Tests

Assignee

Nobody

Reported

2011-09-07 20:20 PDT

Modified

2011-09-15 07:17 PDT History

CC List

9 users Show

URL

Keywords

Depends on

Blocks