TestWebKitAPI for now only works on mac and windows. I think it would be good to let it work for chromium port.
Created attachment 106707 [details] patch
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 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 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.
(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.
(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?
(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.
Perhaps it would be best to follow the "config.h" approach of other WebKit projects (WebCore, WebKit2, etc.).
(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.
(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.
Created attachment 106964 [details] use config.h. Mac and Windows to be verified by bots.
Created attachment 106965 [details] use config.h. Mac and Windows to be verified by bots.
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 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>
All reviewed patches have been landed. Closing bug.
(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 :)
btw, it is cool that this runs for Chromium now!