Bug 27551 - Make it possible to build JavaScriptCore as shared library without symbol lists
: Make it possible to build JavaScriptCore as shared library without symbol lists
Status: RESOLVED FIXED
: WebKit
Platform
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
: 67852 72231 76257
: 65470
  Show dependency treegraph
 
Reported: 2009-07-22 11:40 PST by
Modified: 2012-09-21 16:51 PST (History)


Attachments
Patch to make it possible to build JavaScriptCore as shared library on Linux without symbol files. (81.56 KB, patch)
2009-07-22 11:40 PST, Simon Hausmann
no flags Review Patch | Details | Formatted Diff | Diff
Make it possible to build JSC as a shared library without symbol lists (77.13 KB, patch)
2009-07-23 13:03 PST, Kenneth Rohde Christiansen
no flags Review Patch | Details | Formatted Diff | Diff
Build JSC as a shared library for QtWebKit (5.93 KB, patch)
2009-07-23 13:05 PST, Kenneth Rohde Christiansen
no flags Review Patch | Details | Formatted Diff | Diff
Updated patch, rebased against ToT and fixed windows and mac builds. (76.44 KB, patch)
2009-07-28 07:10 PST, Simon Hausmann
no flags Review Patch | Details | Formatted Diff | Diff
Rebased patch to build JavaScriptCore as shared library with Qt (5.94 KB, patch)
2009-07-28 07:11 PST, Simon Hausmann
no flags Review Patch | Details | Formatted Diff | Diff
Updated patch with one MSVC warning fixed and explanation for the other ignored warning (71.96 KB, patch)
2009-07-30 05:46 PST, Simon Hausmann
mrowe: review-
Review Patch | Details | Formatted Diff | Diff
Updated patch with JS_EXPORTDATA renamed to JS_EXPORT_PRIVATE (72.78 KB, patch)
2009-08-13 05:41 PST, Simon Hausmann
no flags Review Patch | Details | Formatted Diff | Diff
Patch to remove unused WEBKIT_EXPORTDATA macro (3.48 KB, patch)
2009-08-14 05:40 PST, Simon Hausmann
no flags Review Patch | Details | Formatted Diff | Diff
Rebased patch to build JSC with JS_EXPORT_PRIVATE to export symbols (72.51 KB, patch)
2009-08-14 05:41 PST, Simon Hausmann
no flags Review Patch | Details | Formatted Diff | Diff
Use JS_EXPORT_PRIVATE to export individual symbols (122.56 KB, patch)
2010-06-16 21:51 PST, Kevin Ollivier
ggaren: review-
Review Patch | Details | Formatted Diff | Diff
Switch to JS_EXPORTED_SYMBOL, various style and port fixes (149.81 KB, patch)
2010-06-17 14:42 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Same as last patch with proposed GTK build fix (150.74 KB, patch)
2010-06-17 17:26 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Patch with another attempt at GTK bot fix (150.74 KB, patch)
2010-06-21 15:11 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Now trying a Windows fix (163.97 KB, patch)
2010-06-23 18:31 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
One more time, with hopefully Win and Qt fixes (203.89 KB, patch)
2010-07-15 14:26 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
One step at a time, this time updating symbols first (102.77 KB, patch)
2010-08-28 17:34 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Style and build fixes to update last patch (124.22 KB, patch)
2010-08-29 17:10 PST, Kevin Ollivier
abarth: review-
Review Patch | Details | Formatted Diff | Diff
Retry - Step 1 - Define export macros but disable them on all ports. (15.75 KB, patch)
2011-03-06 12:49 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Retry - Step 1 re-submit w/attempts at GTK, Qt and Win build fixes. (17.26 KB, patch)
2011-03-06 17:52 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Retry - Step 1 another attempt at GTK, Qt and Win build fixes. (16.27 KB, patch)
2011-03-06 19:43 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Retry - Step 1 another attempt at Win and Qt build fixes (16.99 KB, patch)
2011-03-07 08:18 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Retry - Step 1 more Win and Qt build fixes, getting closer... (18.93 KB, patch)
2011-03-07 09:39 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Retry - Step 1 more Win and Qt build fixes (21.60 KB, patch)
2011-03-07 11:38 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Retry - Step 1 adding config.h for projects in Tools (29.95 KB, patch)
2011-03-07 13:31 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Retry - Step 1 w/ more config.h additions for projects in Tools (36.52 KB, patch)
2011-03-07 16:52 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Step 2, part 1: Add export symbols, and get the wx port using them. (109.81 KB, patch)
2011-03-25 20:51 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Step 2, part 1, second try: Adding Win build fix and fixing style issues (110.64 KB, patch)
2011-03-26 11:05 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Step 2, part 1, third try: More Win build fixes. (112.23 KB, patch)
2011-03-26 18:19 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Step 2, part 1: this time, just add the macros, don't flip the switch for wx port (109.07 KB, patch)
2011-03-27 10:49 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Step 2, part 1: same as previous patch, just updated to deal with bit rot (110.03 KB, patch)
2011-03-30 19:39 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Fix defines for WTF/JS_EXPORT_PRIVATE in !USE(EXPORT_MACROS) case (3.99 KB, patch)
2011-04-06 15:31 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Add export macros to wtf/text/WTFString.h (14.80 KB, patch)
2011-04-06 17:00 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Add export macros to bytecompiler headers. (1.21 KB, patch)
2011-04-10 10:46 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Add export macros to bytecompiler headers. (1.27 KB, patch)
2011-04-10 11:12 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Add debugger export symbols. (2.02 KB, patch)
2011-05-25 11:55 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Add heap export symbols. (4.53 KB, patch)
2011-05-25 11:59 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Add interpreter export symbols. (1.56 KB, patch)
2011-05-25 12:03 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Add parser export symbols. (1.34 KB, patch)
2011-05-25 12:07 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Add profiler export symbols. (1.52 KB, patch)
2011-05-25 12:32 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Update Heap patch to address bitrot (4.20 KB, patch)
2011-06-08 17:51 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Export macros for JSCell.h (6.57 KB, patch)
2011-06-08 18:09 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Export macros for Identifier.h (3.70 KB, patch)
2011-06-08 18:15 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Export macros for JSGlobalData.h (2.67 KB, patch)
2011-06-08 18:21 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Complete set of JS_EXPORT_PRIVATE and WTF_EXPORT_PRIVATE macro additions (84.77 KB, patch)
2011-07-11 09:51 PST, Kevin Ollivier
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Fix macro defines for WebKitTestRunner by including config.h (1.21 KB, patch)
2011-07-11 21:49 PST, Kevin Ollivier
webkit-ews: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
New fix for WebKitTestRunner generated files that makes sure config.h is always the first header included. (1.20 KB, patch)
2011-07-12 09:35 PST, Kevin Ollivier
no flags Review Patch | Details | Formatted Diff | Diff
Updated complete symbols and config.h fix patch. (90.05 KB, patch)
2011-08-10 16:39 PST, Kevin Ollivier
eric: review-
Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2009-07-22 11:40:51 PST
Created an attachment (id=33277) [details]
Patch to make it possible to build JavaScriptCore as shared library on Linux without symbol files.

Currently JavaScriptCore is built as shared library on the Apple Windows and Mac OS X builds.

The symbols used by WebCore and WebKit are exported in JavaScriptCore via separate files, one for MSVC and one for Mac OS X gcc. The files contain the mangled symbol names.

It is cumbersome to maintain these lists and it becomes worse when taking additional compilers into account, like for example g++ on Linux or RVCT for ARM. When a new symbol needs to be exported it is necessary to determine their mangled name for each supported compiler.

This bug tries to track the patches/work needed to replace the separately maintained export lists with the commonly used export macros.


The initial patch merely adds the export macros where necessary and tweaks the Visual studio and Xcode based builds to accept them, while keeping the symbol files in place.


Pending approval for the first patch, the second step is to change the Visual Studio and Xcode builds to fully utilize the macros and delete the symbol files. On Windows this step will require the placement of additional export macros that are not part of the first patch.
------- Comment #1 From 2009-07-23 13:03:13 PST -------
Created an attachment (id=33357) [details]
Make it possible to build JSC as a shared library without symbol lists
------- Comment #2 From 2009-07-23 13:05:30 PST -------
Created an attachment (id=33358) [details]
Build JSC as a shared library for QtWebKit
------- Comment #3 From 2009-07-23 13:06:11 PST -------
Mark Rowe requested the patch being split up into two, to separate out the Qt build system that he is not able to review.

Please mark the first patch as r- as I'm not authorized to do so.
------- Comment #4 From 2009-07-23 13:13:59 PST -------
(From update of attachment 33277 [details])
Clearing review after splitting up the patch
------- Comment #5 From 2009-07-24 07:49:02 PST -------
(From update of attachment 33357 [details])
I have to clear review on this patch as it needs to be rebased against current ToT (some parts fall away) and I've noticed a new build issue on Windows. I'll rework it next week.
------- Comment #6 From 2009-07-24 07:49:26 PST -------
(From update of attachment 33358 [details])
Clearing review on this one, too, as it's not useful without the other patch
------- Comment #7 From 2009-07-28 07:10:40 PST -------
Created an attachment (id=33626) [details]
Updated patch, rebased against ToT and fixed windows and mac builds.
------- Comment #8 From 2009-07-28 07:11:17 PST -------
Created an attachment (id=33627) [details]
Rebased patch to build JavaScriptCore as shared library with Qt
------- Comment #9 From 2009-07-28 08:56:06 PST -------
(From update of attachment 33626 [details])
>         * JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.vcproj: Disable
>         MSVC warning about inconsistent linkage.

These warnings are usually not something you just want to ignore. I think it would be a lot better to fix them.
------- Comment #10 From 2009-07-28 10:20:58 PST -------
I tried to build your patches with rev. 46488, and found some small error:
- unnecessary JS_EXPORTDATA macros for unions
- missing includedir
- missing JS_EXPORTDATA for JSFunction class

I fixed them, and it works on Qt-Linux without additional regressions:
http://gist.github.com/157541
------- Comment #11 From 2009-07-29 00:14:24 PST -------
(In reply to comment #10)
> - unnecessary JS_EXPORTDATA macros for unions
I mean enums of course.
------- Comment #12 From 2009-07-29 08:09:53 PST -------
(From update of attachment 33626 [details])
Clearing review after discussion with Adam Roben on IRC. Will upload a new patch tomorrow with better explanation for the DLL warnings and the removal of 4273.
------- Comment #13 From 2009-07-30 05:46:38 PST -------
Created an attachment (id=33773) [details]
Updated patch with one MSVC warning fixed and explanation for the other ignored warning
------- Comment #14 From 2009-08-06 20:06:00 PST -------
(From update of attachment 33773 [details])
This patch will require buy-in from some of the Mac build experts like Mark Rowe, and whoever does build submissions for Apple these days.  I think it should be possible to work this way, but I remember the export lists being a conscious choice back in the day.  Please make sure you talk to Mark and ask him who the right Apple people to ask are these days.
------- Comment #15 From 2009-08-07 05:14:17 PST -------
Mark, can you have a look at these?
------- Comment #16 From 2009-08-07 13:07:06 PST -------
(From update of attachment 33627 [details])
Should this be marked obsolete?  Assuming so.
------- Comment #17 From 2009-08-07 13:16:47 PST -------
(From update of attachment 33627 [details])
No this one shouldn't be obsolete, but it depends on the other patch. How can I make this relation clear using bugzilla?
------- Comment #18 From 2009-08-08 11:01:58 PST -------
(From update of attachment 33773 [details])
A couple of concerns and comments:
JS_EXPORTDATA is a confusing choice of name when the majority of things being exported are functions.
There are a number of places that an entire class has been marked as being exported even though only a single symbol is currently listed in JavaScriptCore.exp and JavaScriptCore.def.  ByteCodeGenerator, Parser and ParserArena are the three that jump out at me for this reason.
It's not clear why WEBKIT_EXPORTDATA exists.  It doesn't seem to be used anywhere.
------- Comment #19 From 2009-08-13 05:41:23 PST -------
Created an attachment (id=34737) [details]
Updated patch with JS_EXPORTDATA renamed to JS_EXPORT_PRIVATE

This updated patch is rebased against ToT and uses JS_EXPORT_PRIVATE instead of JS_EXPORTDATA, to clarify that the macro is used to export private symbols.
------- Comment #20 From 2009-08-13 05:46:28 PST -------
(From update of attachment 34737 [details])
Clearing review, I forgot two hunks. Oops.
------- Comment #21 From 2009-08-14 05:40:47 PST -------
Created an attachment (id=34831) [details]
Patch to remove unused WEBKIT_EXPORTDATA macro

This patch removes the unused WEBKIT_EXPORTDATA macro.
------- Comment #22 From 2009-08-14 05:41:52 PST -------
Created an attachment (id=34832) [details]
Rebased patch to build JSC with JS_EXPORT_PRIVATE to export symbols
------- Comment #23 From 2009-08-14 05:45:04 PST -------
(In reply to comment #18)
> (From update of attachment 33773 [details] [details])
> A couple of concerns and comments:
> JS_EXPORTDATA is a confusing choice of name when the majority of things being
> exported are functions.

I agree. I've uploaded an updated patch that replaces JS_EXPORTDATA with JS_EXPORT_PRIVATE, for consistency with the existing JS_EXPORT for the public C API.

> There are a number of places that an entire class has been marked as being
> exported even though only a single symbol is currently listed in
> JavaScriptCore.exp and JavaScriptCore.def.  ByteCodeGenerator, Parser and
> ParserArena are the three that jump out at me for this reason.

Yes, it appears a lot simpler and less cluttering to the source code to annotate the class instead of every used function with the export macro.

> It's not clear why WEBKIT_EXPORTDATA exists.  It doesn't seem to be used
> anywhere.

Well spotted :). I've attached a patch to remove this macro alltogether.
------- Comment #24 From 2009-09-02 02:41:53 PST -------
These are marked for review by Mark.  He's a busy guy, you might try tracking him down via #webkit.
------- Comment #25 From 2009-10-06 03:51:32 PST -------
(From update of attachment 34832 [details])
Clearning review as these patches require rebasing by now.
------- Comment #26 From 2010-05-31 11:20:55 PST -------
(In reply to comment #23)
> > There are a number of places that an entire class has been marked as being
> > exported even though only a single symbol is currently listed in
> > JavaScriptCore.exp and JavaScriptCore.def.  ByteCodeGenerator, Parser and
> > ParserArena are the three that jump out at me for this reason.
> 
> Yes, it appears a lot simpler and less cluttering to the source code to annotate the class instead of every used function with the export macro.

"Less cluttering" perhaps, but it ends up exporting things that we might not want people to depend on. Is it really best to expert an entire class when we really want to export only a single function?
------- Comment #27 From 2010-05-31 11:50:58 PST -------
(In reply to comment #26)
> (In reply to comment #23)
> > > There are a number of places that an entire class has been marked as being
> > > exported even though only a single symbol is currently listed in
> > > JavaScriptCore.exp and JavaScriptCore.def.  ByteCodeGenerator, Parser and
> > > ParserArena are the three that jump out at me for this reason.
> > 
> > Yes, it appears a lot simpler and less cluttering to the source code to annotate the class instead of every used function with the export macro.
> 
> "Less cluttering" perhaps, but it ends up exporting things that we might not want people to depend on. Is it really best to expert an entire class when we really want to export only a single function?

Personally, I would got for exporting the individual functions needed.
------- Comment #28 From 2010-06-01 11:19:44 PST -------
(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #23)
> > > > There are a number of places that an entire class has been marked as being
> > > > exported even though only a single symbol is currently listed in
> > > > JavaScriptCore.exp and JavaScriptCore.def.  ByteCodeGenerator, Parser and
> > > > ParserArena are the three that jump out at me for this reason.
> > > 
> > > Yes, it appears a lot simpler and less cluttering to the source code to annotate the class instead of every used function with the export macro.
> > 
> > "Less cluttering" perhaps, but it ends up exporting things that we might not want people to depend on. Is it really best to expert an entire class when we really want to export only a single function?
> 
> Personally, I would got for exporting the individual functions needed.

I agree with this, but I wonder if it would be an acceptable compromise to get the class export going and come back and tweak the exports later. Those ports that want more control can continue to use the export symbol files as they do now. 

I'll certainly be happy to look at ByteCodeGenerator, Parser and ParserArena, and modernize this patch, but going through every function in the JSCore library and comparing it against the export symbols file is a project and in the meantime, ports like wx are unable to expose the JSCore API to its users (or to tools like DRT) without creating some sort of wrapper API, which seems a lot of trouble to go to just in order to keep users away from certain functions. Thoughts?
------- Comment #29 From 2010-06-16 21:51:32 PST -------
Created an attachment (id=58961) [details]
Use JS_EXPORT_PRIVATE to export individual symbols

I built wx with this successfully but I wasn't able to build the Win port because I'm using 2008 and it appears the build scripts for Win don't support that yet. So probably a good idea to let the bots chew on this patch first. 

Overall, it's a very big patch but almost all of it is just adding JS_EXPORT_PRIVATE to various symbols. I also include the wx build changes, though they weren't much. The main thing to look at for review is the config.h file changes, and for a couple of the classes like String and AtomicString, I had to add explicit copy constructors and operator= methods so that I could specify the correct linkage for them. Other than that, I pretty much just prepended JS_EXPORT_PRIVATE to everything that some other library was asking for.
------- Comment #30 From 2010-06-16 21:53:08 PST -------
Attachment 58961 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
Last 3072 characters of output:
150:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/runtime/JSCell.h:77:  Use 0 instead of NULL.  [readability/null] [4]
JavaScriptCore/runtime/JSCell.h:78:  Use 0 instead of NULL.  [readability/null] [4]
JavaScriptCore/wtf/text/WTFString.h:89:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
JavaScriptCore/wtf/text/WTFString.h:109:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
JavaScriptCore/wtf/text/WTFString.h:110:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
JavaScriptCore/wtf/text/WTFString.h:213:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
JavaScriptCore/wtf/text/WTFString.h:295:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
JavaScriptCore/wtf/text/WTFString.h:302:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
JavaScriptCore/wtf/text/WTFString.h:303:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
JavaScriptCore/wtf/text/WTFString.h:303:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/text/WTFString.h:303:  More than one command on the same line in if  [whitespace/parens] [4]
JavaScriptCore/wtf/unicode/Collator.h:42:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
JavaScriptCore/wtf/CurrentTime.h:42:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
JavaScriptCore/wtf/RandomNumber.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
JavaScriptCore/wtf/FastMalloc.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
JavaScriptCore/wtf/text/AtomicString.h:46:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
JavaScriptCore/wtf/text/AtomicString.h:47:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
JavaScriptCore/wtf/text/AtomicString.h:60:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
JavaScriptCore/wtf/text/AtomicString.h:117:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
JavaScriptCore/wtf/text/AtomicString.h:117:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/text/AtomicString.h:117:  More than one command on the same line in if  [whitespace/parens] [4]
JavaScriptCore/wtf/text/CString.h:61:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
JavaScriptCore/runtime/InitializeThreading.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
JavaScriptCore/wtf/RefCountedLeakCounter.h:29:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 25 in 72 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #31 From 2010-06-16 22:01:29 PST -------
Attachment 58961 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3273248
------- Comment #32 From 2010-06-16 22:27:37 PST -------
Attachment 58961 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3268239
------- Comment #33 From 2010-06-16 22:54:00 PST -------
Attachment 58961 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3309251
------- Comment #34 From 2010-06-17 01:03:28 PST -------
Attachment 58961 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3324280
------- Comment #35 From 2010-06-17 11:18:08 PST -------
I don't think it's good to have both JS_EXPORT_PRIVATE and JS_EXPORTDATA. It's not immediately clear which you should use where.

JS_EXPORT_PRIVATE is a confusing phrase because, read literally, it's an oxymoron: things that are private are things that are *not* exported.

Maybe standardizing on something like "JS_EXPORTED_SYMBOL" would be better.

If this patch made symbol export work the same on all platforms, so that all you had to do when exporting a symbol was to add JS_EXPORTED_SYMBOL to it, without updating any .exp or .def files, I'd be really excited about it. But as it stands, it's just global clutter and more maintenance work for the core maintainers of JavaScriptCore, with no benefit to them. As a result, I'm inclined to say 'no' to this patch.
------- Comment #36 From 2010-06-17 11:56:43 PST -------
(In reply to comment #35)
> I don't think it's good to have both JS_EXPORT_PRIVATE and JS_EXPORTDATA. It's not immediately clear which you should use where.

I did plan on removing them but this patch was big enough as it was and I was hoping to do it as a separate step. However, I can adjust the patch to move everything to one symbol as you suggest. 

> JS_EXPORT_PRIVATE is a confusing phrase because, read literally, it's an oxymoron: things that are private are things that are *not* exported.
>
> Maybe standardizing on something like "JS_EXPORTED_SYMBOL" would be better.

I'll go ahead and change this.

> If this patch made symbol export work the same on all platforms, so that all you had to do when exporting a symbol was to add JS_EXPORTED_SYMBOL to it, without updating any .exp or .def files, I'd be really excited about it. 

It doesn't do that because I'm not an expert on all the other ports' build systems, and this isn't a simple matter of just copying and pasting a file path. If you think about it, though, moving Mac, or Win, or any other port to this is probably not more than a couple hours work. If this patch is not landed, someone will have to do all this work anyway, and someone will have to continue to maintain the .def and .exp files anyway, so I think it would make sense from a maintenance perspective for each port to put in a small amount of effort to move to this.

>But as it stands, it's just global clutter and more maintenance work for the core maintainers of JavaScriptCore, with no benefit to them. As a result, I'm inclined to say 'no' to this patch.

Well, the whole reason I even delved into doing this was because the commentary on this patch so far suggests that this approach is the only way that would be accepted to move us away from using .def and .exp files. It was a lot of work to do, and wx needs it to be able to use JSCore as a shared library, which it needs to get DumpRenderTree working again. 

I'm working on cleaning up the patch now with the issues reported by the bots and with your suggestions, and I hope in the next iteration you'll reconsider.
------- Comment #37 From 2010-06-17 11:58:56 PST -------
On Linux we do not have exp of def files so we will need a patch like this in order to build JSC as a dynamic library for Qt, and we really want to do that.
------- Comment #38 From 2010-06-17 14:42:36 PST -------
Created an attachment (id=59038) [details]
Switch to JS_EXPORTED_SYMBOL, various style and port fixes

Now all the JS internal exports are controlled using a single define, JS_EXPORTED_SYMBOL. The rest of the changes from the last patch are all fixes for build and style bot problems. 

To me, most of the remaining style issues look like either false alarms or really issues that should have a separate bug filed. (e.g. the warnings about DEFINE_GLOBAL, are those legit?)

I've made sure the Mac port builds, and addressed the GTK issue I saw on the EWS bot. Hopefully these changes will fix the Win build as well, but if not, how do I get to the actual files that have the compilation errors for the Win EWS bot?
------- Comment #39 From 2010-06-17 14:46:05 PST -------
Attachment 59038 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
JavaScriptCore/runtime/JSCell.h:76:  Use 0 instead of NULL.  [readability/null] [4]
JavaScriptCore/runtime/JSCell.h:77:  Use 0 instead of NULL.  [readability/null] [4]
JavaScriptCore/wtf/FastMalloc.h:63:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/FastMalloc.h:97:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:98:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:99:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:100:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:101:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:102:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:103:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:104:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:125:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
JavaScriptCore/wtf/FastMalloc.h:166:  One space before end of line comments  [whitespace/comments] [5]
WARNING: File exempt from style guide. Skipping: "WebCore/platform/gtk/gtk2drawing.c"
JavaScriptCore/wtf/text/StringStatics.cpp:52:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:53:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:54:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:55:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:56:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:57:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:58:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 20 in 90 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #40 From 2010-06-17 16:36:34 PST -------
Attachment 59038 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3317325
------- Comment #41 From 2010-06-17 17:26:52 PST -------
Created an attachment (id=59052) [details]
Same as last patch with proposed GTK build fix
------- Comment #42 From 2010-06-17 17:28:31 PST -------
Attachment 59052 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
JavaScriptCore/runtime/JSCell.h:76:  Use 0 instead of NULL.  [readability/null] [4]
JavaScriptCore/runtime/JSCell.h:77:  Use 0 instead of NULL.  [readability/null] [4]
JavaScriptCore/wtf/FastMalloc.h:63:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/FastMalloc.h:97:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:98:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:99:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:100:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:101:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:102:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:103:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:104:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:166:  One space before end of line comments  [whitespace/comments] [5]
WARNING: File exempt from style guide. Skipping: "WebCore/platform/gtk/gtk2drawing.c"
JavaScriptCore/wtf/text/StringStatics.cpp:52:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:53:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:54:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:55:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:56:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:57:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:58:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 19 in 90 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #43 From 2010-06-17 20:37:12 PST -------
Attachment 59052 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3326297
------- Comment #44 From 2010-06-17 22:12:37 PST -------
Attachment 59052 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3328293
------- Comment #45 From 2010-06-21 15:11:42 PST -------
Created an attachment (id=59298) [details]
Patch with another attempt at GTK bot fix

Anyone with Qt or Win port source trees willing to take a look at what is leading to failures there? I'll try to get these set up on my end, but I need a separate Win build machine here because I've already upgraded to MSVC 2008, and I'm not set up for Qt at all, so it might be a while.
------- Comment #46 From 2010-06-21 15:13:44 PST -------
Attachment 59298 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
JavaScriptCore/runtime/JSCell.h:76:  Use 0 instead of NULL.  [readability/null] [4]
JavaScriptCore/runtime/JSCell.h:77:  Use 0 instead of NULL.  [readability/null] [4]
JavaScriptCore/wtf/FastMalloc.h:63:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/FastMalloc.h:97:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:98:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:99:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:100:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:101:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:102:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:103:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:104:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:166:  One space before end of line comments  [whitespace/comments] [5]
WARNING: File exempt from style guide. Skipping: "WebCore/platform/gtk/gtk2drawing.c"
JavaScriptCore/wtf/text/StringStatics.cpp:52:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:53:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:54:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:55:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:56:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:57:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:58:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 19 in 89 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #47 From 2010-06-22 06:47:23 PST -------
Attachment 59298 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3336581
------- Comment #48 From 2010-06-23 18:31:48 PST -------
Created an attachment (id=59596) [details]
Now trying a Windows fix

This got me building on a Windows VM up to WebKit, but the VM build kept choking on "out of memory" linker errors, so I don't know if this fix will get us building all the way. 

I've also discovered the Qt bot's problem - some of their moc-generated sources include files that indirectly include JSCore sources, but they do not include config.h from anywhere. Not sure what the right way to fix that would be. Anyone on the Qt side have suggestions?
------- Comment #49 From 2010-06-23 18:35:10 PST -------
Attachment 59596 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
JavaScriptCore/runtime/JSCell.h:76:  Use 0 instead of NULL.  [readability/null] [4]
JavaScriptCore/runtime/JSCell.h:77:  Use 0 instead of NULL.  [readability/null] [4]
JavaScriptCore/wtf/FastMalloc.h:63:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/FastMalloc.h:97:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:98:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:99:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:100:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:101:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:102:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:103:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:104:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:166:  One space before end of line comments  [whitespace/comments] [5]
WARNING: File exempt from style guide. Skipping: "WebCore/platform/gtk/gtk2drawing.c"
JavaScriptCore/wtf/text/StringStatics.cpp:52:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:53:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:54:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:55:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:56:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:57:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:58:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 19 in 90 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #50 From 2010-06-24 12:42:03 PST -------
Okay, so I'm basically stuck on the Win build because the build bot is not applying the patch for some reason, though applying the patch to the local tree on my Win box works fine, and my Win VM apparently needs more memory than 3GB total to get through linking WebKit and I can't allocate that much right now. 

On the Qt front, I'm just waiting to hear what the best idea would be for handling the issue with moc_XYZ.cpp files not including config.h. 

I'd really, really appreciate any help as I'm concerned about this patch bit-rotting due to its large size.
------- Comment #51 From 2010-06-29 13:11:55 PST -------
(In reply to comment #37)
> On Linux we do not have exp of def files so we will need a patch like this in order to build JSC as a dynamic library for Qt, and we really want to do that.

Are you saying that you haven't made exp or def files yet, or that Linux just doesn't support them?

GCC certainly supports them.
------- Comment #52 From 2010-06-29 13:30:46 PST -------
(In reply to comment #51)
> (In reply to comment #37)
> > On Linux we do not have exp of def files so we will need a patch like this in order to build JSC as a dynamic library for Qt, and we really want to do that.
> 
> Are you saying that you haven't made exp or def files yet, or that Linux just doesn't support them?
> 
> GCC certainly supports them.

Last I looked I didn't find that support for Linux, but support for something similar that for some reason couldn't be used. Do you have any link to the documentation mentioning this?
------- Comment #53 From 2010-06-29 22:58:50 PST -------
(In reply to comment #50)
> On the Qt front, I'm just waiting to hear what the best idea would be for handling the issue with moc_XYZ.cpp files not including config.h. 

The solution to this problem is this:

   1) Foo.h declares a QObject subclass and has a Q_OBJECT macro
   2) Foo.cpp implements the class
   3) The build system runs moc on Foo.h and generates moc_Foo.cpp
   4) moc_Foo.cpp is compiled separately

If however you change Foo.cpp to include "moc_Foo.cpp" at the end of the file (see WebKit/qt/Api/qwebpage.cpp for example), then the build system will automatically skip step number 4). That means the moc generated code is compiled as part of Foo.cpp, which has the config.h inclusion at the top of the file.
------- Comment #54 From 2010-07-13 11:53:19 PST -------
(In reply to comment #53)
> (In reply to comment #50)
> > On the Qt front, I'm just waiting to hear what the best idea would be for handling the issue with moc_XYZ.cpp files not including config.h. 
> 
> The solution to this problem is this:
> 
>    1) Foo.h declares a QObject subclass and has a Q_OBJECT macro
>    2) Foo.cpp implements the class
>    3) The build system runs moc on Foo.h and generates moc_Foo.cpp
>    4) moc_Foo.cpp is compiled separately
> 
> If however you change Foo.cpp to include "moc_Foo.cpp" at the end of the file (see WebKit/qt/Api/qwebpage.cpp for example), then the build system will automatically skip step number 4). That means the moc generated code is compiled as part of Foo.cpp, which has the config.h inclusion at the top of the file.

Thanks, this works fine! Sorry for the late reply, I've been meaning to put up a new patch but I finally got a Windows build linking WebCore and I had to add quite a few more Windows fixes. (A lot of them are related to the current need to build the WTF String .cpp files in each project that uses them.) I've finally sorted those issues out, but now I need to re-test the patch with latest trunk, so hopefully I'll have an updated patch out soon...
------- Comment #55 From 2010-07-15 14:26:12 PST -------
Created an attachment (id=61720) [details]
One more time, with hopefully Win and Qt fixes

This is probably going to be my last shot on this. It's taking too long to keep maintaining and updating a patch this huge even using git and several times on Win I've run into issues like having to redo the Windows project changes manually because of conflicts with the diffs. Hopefully it will apply to the Win bots and get a shot at building this time.
------- Comment #56 From 2010-07-15 14:29:02 PST -------
Attachment 61720 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
Last 3072 characters of output:
 before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:98:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:99:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:100:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:101:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:102:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:103:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:104:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:166:  One space before end of line comments  [whitespace/comments] [5]
WARNING: File exempt from style guide. Skipping: "WebCore/platform/gtk/gtk2drawing.c"
WebKitTools/DumpRenderTree/cg/ImageDiffCG.cpp:29:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
JavaScriptCore/API/tests/testapi.c:26:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
JavaScriptCore/API/tests/JSNode.c:26:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
JavaScriptCore/API/tests/minidom.c:27:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
JavaScriptCore/API/tests/JSNodeList.c:26:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:52:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:53:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:54:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:55:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:56:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:57:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:58:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 25 in 102 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #57 From 2010-07-15 14:42:12 PST -------
I’ve done large patches like this before.

A good way to get a change like this landed without having to maintain a giant patch is to get agreement on the approach, put in placeholder macros that do nothing, and then get patches landed that deploy the macros in various parts of the code, with the macros not yet functioning.

Then the last patch will just be throwing the switch and turning this on. That one is hard to make work successfully, but easy to merge.
------- Comment #58 From 2010-07-15 15:22:11 PST -------
(In reply to comment #57)
> I’ve done large patches like this before.
> 
> A good way to get a change like this landed without having to maintain a giant patch is to get agreement on the approach, put in placeholder macros that do nothing, and then get patches landed that deploy the macros in various parts of the code, with the macros not yet functioning.
> 
> Then the last patch will just be throwing the switch and turning this on. That one is hard to make work successfully, but easy to merge.

I considered that, but too late, unfortunately. If I had realized how this would turn out, I would have waited to merge JS_EXPORTDATA and JS_EXPORTED_SYMBOL, because in order to make JS_EXPORTED_SYMBOL a stub JS_EXPORTDATA needs to stay in the code. :( It can be 'undone', but I'd have to either go through and find JS_EXPORTDATA symbols one-by-one and rename them, or go back about 6 revisions of the patch and then reapply all the fixes made in those later revisions.

The big sticking point with maintenance is the need to alter the Win project files, and that is needed because we have a system set up that is copying the WTF *String.cpp files into a place where WebCore and other projects can compile them. I don't really understand the reasoning for this, but if this could be fixed independently by someone, then it would remove a lot of the issues I've been dealing with on the Win side (e.g. the patches not applying on the EWS bot, the need to undo and redo the Win project changes whenever someone changes the project files, etc.)
------- Comment #59 From 2010-07-15 15:23:47 PST -------
> It can be 'undone', but I'd have to either go through and find JS_EXPORTDATA symbols one-by-one and rename them...

rename them from JS_EXPORTED_SYMBOL back to JS_EXPORTDATA, I meant to say.
------- Comment #60 From 2010-07-15 18:51:34 PST -------
Attachment 61720 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3557030
------- Comment #61 From 2010-07-16 11:35:10 PST -------
So the Chromium problem is a simple JS_EXPORTDATA -> JS_EXPORTED_SYMBOL change in their DRT config.h header, but again the Win bot can't apply the Win project changes to the patch, and I don't think that will change if I update a new patch with the Chromium fix. I really have to move on to addressing some other problems for now, so I'll get back to this when I can, but if someone wants to give it a shot before then, the place to start from would probably be the "Use JS_EXPORT_PRIVATE to export individual symbols" patch. 

Note that having gone through and made fixes for most of the ports, one thing I would probably do if I had to do it again is have some separate JSCore header for the defines. (And maybe a WTF one too, for projects only including WTF headers.) Maybe JSConfig.h and WTFConfig.h? Then in config.h or similar project headers, I'd include the JSCore / WTF header. This way, JS_EXPORTED_SYMBOL is only defined in one place rather than having a lot of copied and pasted re-definitions and would be defined based on a combination of JS_EXPORT + BUILDING_<JavaScriptCore/WTF> values.
------- Comment #62 From 2010-08-14 17:04:48 PST -------
I would advice to skip the style parts from the patch for easier review.
------- Comment #63 From 2010-08-28 17:34:07 PST -------
Created an attachment (id=65843) [details]
One step at a time, this time updating symbols first

Trying the approach Darin suggested of updating all the macros, but not switching it on for ports yet so we can address the build system issues in later patches.
------- Comment #64 From 2010-08-28 17:36:05 PST -------
Attachment 65843 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/runtime/JSWrapperObject.h:31:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
JavaScriptCore/runtime/JSCell.h:77:  Use 0 instead of NULL.  [readability/null] [4]
JavaScriptCore/runtime/JSCell.h:78:  Use 0 instead of NULL.  [readability/null] [4]
JavaScriptCore/runtime/StringObject.h:29:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
JavaScriptCore/wtf/text/WTFString.h:300:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
JavaScriptCore/wtf/unicode/Collator.h:42:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
JavaScriptCore/wtf/CurrentTime.h:42:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
JavaScriptCore/wtf/RandomNumber.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
JavaScriptCore/wtf/FastMalloc.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
JavaScriptCore/wtf/text/AtomicString.h:44:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
JavaScriptCore/wtf/text/AtomicString.h:45:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
JavaScriptCore/wtf/text/AtomicString.h:58:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
JavaScriptCore/wtf/text/AtomicString.h:115:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
JavaScriptCore/wtf/text/AtomicString.h:115:  More than one command on the same line in if  [whitespace/parens] [4]
JavaScriptCore/wtf/text/CString.h:61:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
JavaScriptCore/runtime/InitializeThreading.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
JavaScriptCore/wtf/RefCountedLeakCounter.h:29:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 17 in 74 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #65 From 2010-08-28 17:36:54 PST -------
Also, note that with gcc, I had to export a few entire classes like JSObject, because if anything in WebCore derived from them, we'd get undefined symbol errors for the class' vtable and typeinfo. I found it documented that these aren't made public unless the class itself is exported. I only did it when necessary to resolve those issues, though.
------- Comment #66 From 2010-08-28 17:46:34 PST -------
Attachment 65843 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3884061
------- Comment #67 From 2010-08-28 17:54:18 PST -------
Attachment 65843 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3839083
------- Comment #68 From 2010-08-28 18:27:07 PST -------
Attachment 65843 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3834083
------- Comment #69 From 2010-08-28 18:29:32 PST -------
Yipes, sorry for the patch spam. :( I had forgotten how far back this patch was, and what all was missing from it. I'll go through and reapply some of the fixes and re-submit.

BTW, for WTF headers, I know Platform.h is pretty much available when building anything, but do ports include / copy over all WTF headers or only specific ones?
------- Comment #70 From 2010-08-28 21:25:28 PST -------
Attachment 65843 [details] did not build on win:
Build output: http://queues.webkit.org/results/3854087
------- Comment #71 From 2010-08-29 09:41:07 PST -------
> BTW, for WTF headers, I know Platform.h is pretty much available when building anything, but do ports include / copy over all WTF headers or only specific ones?

In WebKit2 the wtf headers are included in the "normal" way (in contrast to WebCore and JavaScriptCore). Maybe this is the case everywhere.
------- Comment #72 From 2010-08-29 17:10:41 PST -------
Created an attachment (id=65865) [details]
Style and build fixes to update last patch

I think this patch will work for all the ports, but let's see first. :)
------- Comment #73 From 2010-08-29 17:15:47 PST -------
Attachment 65865 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/wtf/FastMalloc.h:101:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:102:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:103:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:104:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:105:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:106:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:107:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:108:  One space before end of line comments  [whitespace/comments] [5]
JavaScriptCore/wtf/FastMalloc.h:171:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 9 in 75 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #74 From 2010-08-29 20:39:24 PST -------
Attachment 65865 [details] did not build on win:
Build output: http://queues.webkit.org/results/3822118
------- Comment #75 From 2010-12-23 00:18:43 PST -------
(From update of attachment 65865 [details])
This patch has been up for review for months and doesn't build on Windows.
------- Comment #76 From 2011-03-06 12:49:11 PST -------
Created an attachment (id=84895) [details]
Retry - Step 1 - Define export macros but disable them on all ports.

Well, I'm at it again. :) So after thinking about it, I'd like to tackle this problem in the following stages:

1) Define the export macros, but leave them disabled on all ports (i.e. #define WTF/JS_EXPORT_PRIVATE  ). I've defined WTF_EXPORT_PRIVATE on the exported functions in Assertions.h to make sure that we hit build errors in each port and project if these macros are not defined. This way we can sort the build errors with the macros not being defined without having to first update the exports list each time we hit a build error on a port, and also it makes the patch easier to review and digest by making sure there's very little noise in the patch. 

2) Get the wx port building. This patch will add the massive list of exports, but as long as the build errors in #1 should addressed, all other ports should be unaffected. As the wx port maintainer, I will confirm that it doesn't break the wx port, and so if someone could just look over this to make sure things are in good order and r+ if so, that would be great.

3) Work out the build issues for each port, starting with Apple Mac and Windows. If they want to get a jump on tackling the issue, any port should be able to work out the issues themselves by defining WTF_USE_EXPORT_MACROS 1 on their port and seeing what breaks.

Hopefully this approach will make it easier to get this problem tackled once and for all! Please let me know what you think.
------- Comment #77 From 2011-03-06 12:50:38 PST -------
Attachment 84895 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSBase.h', u'Sou..." exit_code: 1

Source/JavaScriptCore/wtf/Assertions.h:151:  The parameter name "channel" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/Assertions.h:152:  The parameter name "channel" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #78 From 2011-03-06 13:23:14 PST -------
Attachment 84895 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8101355
------- Comment #79 From 2011-03-06 14:22:38 PST -------
Attachment 84895 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8105304
------- Comment #80 From 2011-03-06 14:28:05 PST -------
Attachment 84895 [details] did not build on win:
Build output: http://queues.webkit.org/results/8105305
------- Comment #81 From 2011-03-06 15:07:35 PST -------
Attachment 84895 [details] did not build on win:
Build output: http://queues.webkit.org/results/8102386
------- Comment #82 From 2011-03-06 17:52:29 PST -------
Created an attachment (id=84903) [details]
Retry - Step 1 re-submit w/attempts at GTK, Qt and Win build fixes.

New try with build fixes for the ports broken by the patch. Not really sure if I should address the style bot issues.... It would seem inconsistent to remove the variable name from that one variable while leaving all the others in there. 

Also, in case anyone familiar with Source/WebKit/cf/WebCoreSupport/WebInspectorClientCF.cpp is reading, is there any reason the file doesn't just do:

#if defined(WIN32) || defined(_WIN32)
#include "config.h"
#else
// set defines needed on Mac due to missing config.h
#endif

Most of the defines set in there are for the Win32 case, which from the comment should have access to config.h, it is only the Mac port that is without it. I could set a different bug for that if there's interest in having it handled but doing it in this patch isn't appropriate.
------- Comment #83 From 2011-03-06 17:55:25 PST -------
Attachment 84903 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSBase.h', u'Sou..." exit_code: 1

Source/JavaScriptCore/wtf/Assertions.h:151:  The parameter name "channel" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/Assertions.h:152:  The parameter name "channel" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #84 From 2011-03-06 18:43:53 PST -------
Attachment 84903 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8104367
------- Comment #85 From 2011-03-06 18:58:58 PST -------
Attachment 84903 [details] did not build on win:
Build output: http://queues.webkit.org/results/8107201
------- Comment #86 From 2011-03-06 19:33:22 PST -------
Attachment 84903 [details] did not build on win:
Build output: http://queues.webkit.org/results/8107206
------- Comment #87 From 2011-03-06 19:43:59 PST -------
Created an attachment (id=84910) [details]
Retry - Step 1 another attempt at GTK, Qt and Win build fixes.

Looks like I moved around a bit too much in WebKit2/config.h while trying to make sure that FastMalloc.h was included only after JS/WTF_EXPORT_PRIVATE macros were defined.
------- Comment #88 From 2011-03-06 19:46:31 PST -------
Attachment 84910 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSBase.h', u'Sou..." exit_code: 1

Source/JavaScriptCore/wtf/Assertions.h:151:  The parameter name "channel" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/Assertions.h:152:  The parameter name "channel" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #89 From 2011-03-06 20:37:28 PST -------
Attachment 84910 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8103456
------- Comment #90 From 2011-03-06 21:02:44 PST -------
Attachment 84910 [details] did not build on win:
Build output: http://queues.webkit.org/results/8101427
------- Comment #91 From 2011-03-06 21:37:02 PST -------
Attachment 84910 [details] did not build on win:
Build output: http://queues.webkit.org/results/8107231
------- Comment #92 From 2011-03-07 08:18:50 PST -------
Created an attachment (id=84946) [details]
Retry - Step 1 another attempt at Win and Qt build fixes
------- Comment #93 From 2011-03-07 08:21:00 PST -------
Attachment 84946 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSBase.h', u'Sou..." exit_code: 1

Source/JavaScriptCore/wtf/Assertions.h:151:  The parameter name "channel" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/Assertions.h:152:  The parameter name "channel" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #94 From 2011-03-07 08:59:20 PST -------
Attachment 84946 [details] did not build on win:
Build output: http://queues.webkit.org/results/8103565
------- Comment #95 From 2011-03-07 09:19:19 PST -------
Attachment 84946 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8105497
------- Comment #96 From 2011-03-07 09:39:53 PST -------
Created an attachment (id=84952) [details]
Retry - Step 1 more Win and Qt build fixes, getting closer...
------- Comment #97 From 2011-03-07 09:44:11 PST -------
Attachment 84952 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSBase.h', u'Sou..." exit_code: 1

Source/JavaScriptCore/wtf/Assertions.h:151:  The parameter name "channel" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/Assertions.h:152:  The parameter name "channel" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #98 From 2011-03-07 10:26:32 PST -------
Attachment 84952 [details] did not build on win:
Build output: http://queues.webkit.org/results/8107352
------- Comment #99 From 2011-03-07 11:02:12 PST -------
Attachment 84952 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8107359
------- Comment #100 From 2011-03-07 11:38:06 PST -------
Created an attachment (id=84962) [details]
Retry - Step 1 more Win and Qt build fixes

Sorry for the patch spam, everyone, but it seems on these smaller projects in tools there is often not a config.h so that's why the build keeps breaking as we get down into Tools.
------- Comment #101 From 2011-03-07 11:39:45 PST -------
Attachment 84962 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSBase.h', u'Sou..." exit_code: 1

Tools/WebKitTestRunner/qt/TestControllerQt.cpp:27:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Tools/WebKitTestRunner/qt/TestInvocationQt.cpp:26:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Tools/WebKitTestRunner/qt/main.cpp:27:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Tools/WebKitTestRunner/qt/PlatformWebViewQt.cpp:27:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Tools/WebKitTestRunner/WebKitTestRunnerPrefix.h:33:  #ifndef header guard has wrong style, please use: WTF_WebKitTestRunnerPrefix_h  [build/header_guard] [5]
Source/JavaScriptCore/wtf/Assertions.h:151:  The parameter name "channel" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/Assertions.h:152:  The parameter name "channel" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 7 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #102 From 2011-03-07 12:22:59 PST -------
Attachment 84962 [details] did not build on win:
Build output: http://queues.webkit.org/results/8098825
------- Comment #103 From 2011-03-07 13:31:51 PST -------
Created an attachment (id=84975) [details]
Retry - Step 1 adding config.h for projects in Tools
------- Comment #104 From 2011-03-07 13:34:35 PST -------
Attachment 84975 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSBase.h', u'Sou..." exit_code: 1

Tools/WebKitTestRunner/config.h:25:  #ifndef header guard has wrong style, please use: WTF_config_h  [build/header_guard] [5]
Source/JavaScriptCore/wtf/Assertions.h:151:  The parameter name "channel" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/Assertions.h:152:  The parameter name "channel" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/WebKitAPITest/config.h:25:  #ifndef header guard has wrong style, please use: WTF_config_h  [build/header_guard] [5]
Total errors found: 4 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #105 From 2011-03-07 14:02:41 PST -------
Attachment 84962 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8103610
------- Comment #106 From 2011-03-07 14:45:28 PST -------
Attachment 84975 [details] did not build on win:
Build output: http://queues.webkit.org/results/8105563
------- Comment #107 From 2011-03-07 15:19:06 PST -------
Attachment 84975 [details] did not build on win:
Build output: http://queues.webkit.org/results/8102661
------- Comment #108 From 2011-03-07 16:12:08 PST -------
Attachment 84975 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8107440
------- Comment #109 From 2011-03-07 16:52:16 PST -------
Created an attachment (id=84999) [details]
Retry - Step 1 w/ more config.h additions for projects in Tools
------- Comment #110 From 2011-03-07 16:54:22 PST -------
Attachment 84999 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSBase.h', u'Sou..." exit_code: 1

Tools/WebKitTestRunner/config.h:25:  #ifndef header guard has wrong style, please use: WTF_config_h  [build/header_guard] [5]
Source/JavaScriptCore/wtf/Assertions.h:151:  The parameter name "channel" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/Assertions.h:152:  The parameter name "channel" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/WebKitAPITest/config.h:25:  #ifndef header guard has wrong style, please use: WTF_config_h  [build/header_guard] [5]
Total errors found: 4 in 53 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #111 From 2011-03-09 14:00:33 PST -------
So it looks like I've finally got a patch with everything building. :) Please let me know what you think! With this step complete, we should be able to start adding the export macros to the headers and updating ports to use them one at a time.
------- Comment #112 From 2011-03-13 18:25:27 PST -------
(From update of attachment 84999 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=84999&action=review

Concept seems fine and execution seems OK.

The naming is a little inconsistent. We have JS_EXPORTDATA but JS_EXPORT_PRIVATE, so two different styles in how to deal with multiple words.

> Tools/WebKitAPITest/HostWindow.cpp:27
> +#include "config.h"
> +
>  #include "HostWindow.h"

Normally our style is to not have a blank line between the include of config.h and the include of the file's own header. We could have a blank line if the first header is not the file’s own.

>> Tools/WebKitAPITest/config.h:25
>> +#ifndef WebKitAPITests_config_h
> 
> #ifndef header guard has wrong style, please use: WTF_config_h  [build/header_guard] [5]

False error from the style tool. We should file another bug asking for it to be fixed.

> Tools/WebKitAPITest/config.h:31
> +#if USE(EXPORT_MACROS)

Should have a blank line after this since we have a blank line before the #else and the #endif.

> Tools/WebKitAPITest/config.h:41
> +#else /* !USE(EXPORT_MACROS) */

I’m concerned that the boilerplate section of the config files are so long! Seems like plenty of room for error. Is there any way to avoid having so much boilerplate?

>> Source/JavaScriptCore/wtf/Assertions.h:151
>> +WTF_EXPORT_PRIVATE void WTFLog(WTFLogChannel* channel, const char* format, ...) WTF_ATTRIBUTE_PRINTF(2, 3);
> 
> The parameter name "channel" adds no information, so it should be removed.  [readability/parameter_name] [5]

I agree with the style queue about this.

>> Source/JavaScriptCore/wtf/Assertions.h:152
>> +WTF_EXPORT_PRIVATE void WTFLogVerbose(const char* file, int line, const char* function, WTFLogChannel* channel, const char* format, ...) WTF_ATTRIBUTE_PRINTF(5, 6);
> 
> The parameter name "channel" adds no information, so it should be removed.  [readability/parameter_name] [5]

And here too.
------- Comment #113 From 2011-03-13 20:23:15 PST -------
(In reply to comment #112)
> (From update of attachment 84999 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84999&action=review
> 
> Concept seems fine and execution seems OK.
> 
> The naming is a little inconsistent. We have JS_EXPORTDATA but JS_EXPORT_PRIVATE, so two different styles in how to deal with multiple words.

This is temporary, and it will all end up being JS_EXPORT_PRIVATE in the end. I am leaving this temporarily to not break any port we haven't yet moved to using the export macros.

> > Tools/WebKitAPITest/HostWindow.cpp:27
> > +#include "config.h"
> > +
> >  #include "HostWindow.h"
> 
> Normally our style is to not have a blank line between the include of config.h and the include of the file's own header. We could have a blank line if the first header is not the file’s own.

Okay, will fix before landing.

> >> Tools/WebKitAPITest/config.h:25
> >> +#ifndef WebKitAPITests_config_h
> > 
> > #ifndef header guard has wrong style, please use: WTF_config_h  [build/header_guard] [5]
> 
> False error from the style tool. We should file another bug asking for it to be fixed.

Okay, I'll post a bug about this.

> > Tools/WebKitAPITest/config.h:31
> > +#if USE(EXPORT_MACROS)
> 
> Should have a blank line after this since we have a blank line before the #else and the #endif.

Okay, will fix before landing.

> > Tools/WebKitAPITest/config.h:41
> > +#else /* !USE(EXPORT_MACROS) */
> 
> I’m concerned that the boilerplate section of the config files are so long! Seems like plenty of room for error. Is there any way to avoid having so much boilerplate?

Well, first, the !USE(EXPORT_MACROS) case (and the USE check itself) will disappear once all ports have moved to using it, ideally in a few weeks or so. That being said, what I'd eventually like to do is simply to have some headers like WTFDefines.h, JSDefines.h, etc. that defines the various export macros they use and are simply included by the various config.h files, or maybe even directly by the headers that need them. There is a lot of duplicate code defining the various macros in each config.h, and it'd be nice to just include a header instead of re-defining them over and over.

This and removing JS_EXPORTDATA were things I started trying to handle on the first attempt to address this, but it led to having to figure out the issues with all the build systems at once, and this caused the patch to become unwieldily to maintain as the fixes piled up. So I'd like to try all these things, but once we've got the export macros working in basic form everywhere first.

> >> Source/JavaScriptCore/wtf/Assertions.h:151
> >> +WTF_EXPORT_PRIVATE void WTFLog(WTFLogChannel* channel, const char* format, ...) WTF_ATTRIBUTE_PRINTF(2, 3);
> > 
> > The parameter name "channel" adds no information, so it should be removed.  [readability/parameter_name] [5]
> 
> I agree with the style queue about this.
> 
> >> Source/JavaScriptCore/wtf/Assertions.h:152
> >> +WTF_EXPORT_PRIVATE void WTFLogVerbose(const char* file, int line, const char* function, WTFLogChannel* channel, const char* format, ...) WTF_ATTRIBUTE_PRINTF(5, 6);
> > 
> > The parameter name "channel" adds no information, so it should be removed.  [readability/parameter_name] [5]
> 
> And here too.

Okay, then I'll change them before landing. Many thanks for reviewing this! :)
------- Comment #114 From 2011-03-15 11:10:25 PST -------
Step 1 landed in r81135, thanks!
------- Comment #115 From 2011-03-25 20:51:00 PST -------
Created an attachment (id=87006) [details]
Step 2, part 1: Add export symbols, and get the wx port using them.

And here it is finally, the Big Kahuna patch. :) I've tried to export only the symbols actually used whenever possible, and when I did exported the class itself, it was due to the need to export the vtable, etc. Note however that when I export the specific symbols rather than the class itself, MSVC spits out a ton of warnings, so when we do the Windows port we'll need to either silence those warnings or be a bit more liberal about exporting classes themselves. Also, I used JS_EXPORTDATA in a few instances to keep the ports not using the new export macros building, as mentioned earlier once all ports are moved over that define will be merged with JS_EXPORT_PRIVATE. 

Oh, and I can vouch that it builds and runs on wx fine and there aren't to my knowledge any other reviewers familiar with the wx build system, so please don't hold on the review waiting for someone to verify the wx parts. :)
------- Comment #116 From 2011-03-25 20:52:38 PST -------
Attachment 87006 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Last 3072 characters of output:
ce/JavaScriptCore/wtf/ByteArray.h:83:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/Identifier.h:56:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/Identifier.h:57:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/Identifier.h:58:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/Identifier.h:111:  The parameter name "r" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/StringObject.h:29:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/wtf/FastMalloc.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/PropertyDescriptor.h:50:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/InitializeThreading.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/wtf/MainThread.h:47:  The parameter name "paused" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/text/StringImpl.h:349:  The parameter name "a" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/text/StringBuilder.h:57:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/RandomNumber.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/RegExpObject.h:33:  The parameter name "globalObject" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:92:  The parameter name "dumpsGeneratedCode" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSCell.h:91:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSCell.h:92:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSCell.h:93:  Use 0 or null instead of NULL (even in *comments*).  [readability/null] [4]
Source/JavaScriptCore/runtime/JSCell.h:94:  Use 0 or null instead of NULL (even in *comments*).  [readability/null] [4]
Source/JavaScriptCore/wtf/RefCountedLeakCounter.h:29:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/wtf/ThreadingPrimitives.h:142:  The parameter name "mutex" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 31 in 80 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #117 From 2011-03-25 21:56:25 PST -------
Attachment 87006 [details] did not build on win:
Build output: http://queues.webkit.org/results/8252400
------- Comment #118 From 2011-03-25 23:05:13 PST -------
Attachment 87006 [details] did not build on win:
Build output: http://queues.webkit.org/results/8240957
------- Comment #119 From 2011-03-26 11:05:56 PST -------
Created an attachment (id=87029) [details]
Step 2, part 1, second try: Adding Win build fix and fixing style issues

I'm not going to fix the namespace indentation issues in this patch, as doing so would add hundreds of lines of noise to the patch, and it's already not an easy patch to review. I can fix it before landing though.

Also, what are we going to do about the gtk and Chromium bots? Do we wait, or can I land once everything else is green and just fix any issues that crop up?
------- Comment #120 From 2011-03-26 11:07:04 PST -------
Attachment 87029 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/runtime/JSWrapperObject.h:31:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/StringObject.h:29:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/wtf/FastMalloc.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/InitializeThreading.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/wtf/RandomNumber.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/wtf/RefCountedLeakCounter.h:29:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 6 in 82 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #121 From 2011-03-26 12:39:59 PST -------
Attachment 87029 [details] did not build on win:
Build output: http://queues.webkit.org/results/8255645
------- Comment #122 From 2011-03-26 13:49:19 PST -------
Attachment 87029 [details] did not build on win:
Build output: http://queues.webkit.org/results/8255689
------- Comment #123 From 2011-03-26 18:19:08 PST -------
Created an attachment (id=87051) [details]
Step 2, part 1, third try: More Win build fixes.
------- Comment #124 From 2011-03-26 18:21:45 PST -------
Attachment 87051 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/FastMalloc.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/InitializeThreading.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/JSWrapperObject.h:31:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/wtf/RandomNumber.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/StringObject.h:29:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/wtf/RefCountedLeakCounter.h:29:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 6 in 86 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #125 From 2011-03-26 21:42:28 PST -------
(From update of attachment 87051 [details])
The size of this patch is wrong.

The “flip the switch” patch should be just that, flipping the switch on one platform at a time, with no new macro annotations.

Earlier patches will keep adding the appropriate annotations, but doing so without flipping the switch so there is no immediate effect.
------- Comment #126 From 2011-03-27 10:49:52 PST -------
Created an attachment (id=87077) [details]
Step 2, part 1: this time, just add the macros, don't flip the switch for wx port
------- Comment #127 From 2011-03-27 10:53:17 PST -------
Attachment 87077 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/runtime/JSWrapperObject.h:31:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/StringObject.h:29:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/wtf/FastMalloc.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/InitializeThreading.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/wtf/RandomNumber.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/wtf/RefCountedLeakCounter.h:29:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 6 in 82 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #128 From 2011-03-30 19:39:04 PST -------
Created an attachment (id=87659) [details]
Step 2, part 1: same as previous patch, just updated to deal with bit rot
------- Comment #129 From 2011-03-30 19:44:00 PST -------
Attachment 87659 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/runtime/JSWrapperObject.h:31:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/StringObject.h:29:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/wtf/FastMalloc.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/InitializeThreading.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/wtf/RandomNumber.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/JSCell.h:93:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSCell.h:94:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/RefCountedLeakCounter.h:29:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 8 in 83 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #130 From 2011-04-04 10:05:31 PST -------
Anything I can do to move this patch review along? I realize that everyone's busy, but I'm just wondering if there are some concerns with the approach I'm using, etc. that would cause people to hesitate to review this. I know it's a big patch, but it's simply the macro annotations on symbols needed to allow ports to build. Once this is landed, we can start getting the ports to use them, which I think is the fastest way to work out any kinks in the macros.

Oh, and I will fix the remaining style errors before landing. I can add them if this will speed patch review, but I was concerned the indentation issues would cause a lot of noise in the patch that would complicate review.
------- Comment #131 From 2011-04-06 15:31:12 PST -------
Created an attachment (id=88532) [details]
Fix defines for WTF/JS_EXPORT_PRIVATE in !USE(EXPORT_MACROS) case

Okay, I'll break this down in the hopes of speeding up the review process. This first patch just fixes some cases for !USE(EXPORT_MACROS) where WTF/JS_EXPORT_PRIVATE were erroneously defined. They should only be defined to the appropriate export / import macro when using the export macros.
------- Comment #132 From 2011-04-06 16:39:48 PST -------
(In reply to comment #131)
> Created an attachment (id=88532) [details] [details]
> Fix defines for WTF/JS_EXPORT_PRIVATE in !USE(EXPORT_MACROS) case
> 
> Okay, I'll break this down in the hopes of speeding up the review process. This first patch just fixes some cases for !USE(EXPORT_MACROS) where WTF/JS_EXPORT_PRIVATE were erroneously defined. They should only be defined to the appropriate export / import macro when using the export macros.

First patch landed in r83121, thanks!
------- Comment #133 From 2011-04-06 17:00:09 PST -------
Created an attachment (id=88543) [details]
Add export macros to wtf/text/WTFString.h
------- Comment #134 From 2011-04-06 17:04:40 PST -------
(From update of attachment 88543 [details])
Wow, does it really have to be done on every function like this? I guess I shouldn’t be all that surprised, but still, I’m surprised about inline functions like isHashTableDeletedValue needing this.
------- Comment #135 From 2011-04-06 17:41:56 PST -------
(In reply to comment #134)
> (From update of attachment 88543 [details] [details])
> Wow, does it really have to be done on every function like this? I guess I shouldn’t be all that surprised, but still, I’m surprised about inline functions like isHashTableDeletedValue needing this.

If we don't export the whole class, yes. As for why we need it for inline methods, I haven't found a clear answer (other than that the linker complains about undefined symbols if I don't :), but my guess is that we need it on the inline functions because the WTFString class itself is private, so the compiler probably doesn't have a visible class vtable to bind the inline method to. 

IMHO, at least in terms of WTF, I think it makes a lot of sense to just declare the classes themselves as exported if there's more than a couple methods used. It is supposed to be a helper library, and I don't think there is the intent to discourage use of WTF outside of JSCore, so I would think we'd eventually want there to be a public API for it, at least in principle.

In any case, I'll handle things however you guys think best. I'd just like to see this resolved so I don't have to maintain my own export symbol list on wx just to get DRT going. :)
------- Comment #136 From 2011-04-06 17:43:44 PST -------
(In reply to comment #135)
> (In reply to comment #134)
> > Wow, does it really have to be done on every function like this?
> 
> If we don't export the whole class, yes.

Does exporting the whole class export all the private member functions too?
------- Comment #137 From 2011-04-06 18:01:32 PST -------
(In reply to comment #136)
> (In reply to comment #135)
> > (In reply to comment #134)
> > > Wow, does it really have to be done on every function like this?
> > 
> > If we don't export the whole class, yes.
> 
> Does exporting the whole class export all the private member functions too?

My understanding is that it does, as you might declare the class as a friend. gcc docs recommend using "hidden" visibility to hide any private stuff we don't want exposed, but I don't think there is such an option on Windows, at least, not without using a .def file.
------- Comment #138 From 2011-04-06 18:02:31 PST -------
(In reply to comment #137)
> My understanding is that it does,

OK

> as you might declare the class as a friend

That's backwards. It would matter only if the class declared one of yours as a friend.
------- Comment #139 From 2011-04-06 19:05:47 PST -------
(In reply to comment #138)
> (In reply to comment #137)
> > My understanding is that it does,
> 
> OK
> 
> > as you might declare the class as a friend
> 
> That's backwards. It would matter only if the class declared one of yours as a friend.

Yes, sorry, it was poorly worded. 

In any case, how would you like me to proceed? Keep things as-is with each individual method exported, or switch in this case to exporting the class? We're already exporting a majority of the class, and I think most things that get added will probably need macros assigned to it as well, so exporting the whole class I think would seem to be less maintenance work. However, I'm fine with whatever others decide. Just let me know how you want this handled.
------- Comment #140 From 2011-04-10 10:46:01 PST -------
Created an attachment (id=88953) [details]
Add export macros to bytecompiler headers.

I'm going to start adding the smaller / simpler changes while awaiting a decision on what to do with WTFString.h, so that hopefully we can get some of these changes out of my branch and into the tree.
------- Comment #141 From 2011-04-10 10:48:28 PST -------
Attachment 88953 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:92:  The parameter name "dumpsGeneratedCode" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #142 From 2011-04-10 11:12:52 PST -------
Created an attachment (id=88955) [details]
Patch
------- Comment #143 From 2011-05-11 11:04:38 PST -------
Ping on this. Any ideas on how best to proceed? I would think the bytecompiler headers patch is about as small and simple as it gets, so if there's some issue, please let me know.
------- Comment #144 From 2011-05-23 15:13:55 PST -------
(From update of attachment 88543 [details])
Since WTF_EXPORT_PRIVATE has already been approved, the idea has been OK'd.  Looking at the implementation details of thsi patch, nothing seems awry.  So OK.  rs=me.
------- Comment #145 From 2011-05-23 15:14:21 PST -------
(From update of attachment 88955 [details])
r=me.
------- Comment #146 From 2011-05-23 15:14:49 PST -------
Do we have a buildbot to test this?  We'll need one.  Otherwise this build will break constantly.
------- Comment #147 From 2011-05-24 21:45:53 PST -------
(In reply to comment #146)
> Do we have a buildbot to test this?  We'll need one.  Otherwise this build will break constantly.

The plan is to move all the ports over, starting with wx and then with the Apple Mac port, then Apple Win, and so on. I could do it in a different order if people come in and help me move their port over, though. 

So first we need to get all these macros in the tree, then start switching ports over one-by-one. I don't think it will take much time to move the others over after we have one port working.
------- Comment #148 From 2011-05-24 21:46:41 PST -------
WTFString.h patch landed in r87269, thanks!
------- Comment #149 From 2011-05-25 10:10:45 PST -------
bytecompiler_exports.patch landed in r87304, thanks!
------- Comment #150 From 2011-05-25 11:55:28 PST -------
Created an attachment (id=94823) [details]
Add debugger export symbols.
------- Comment #151 From 2011-05-25 11:59:48 PST -------
Created an attachment (id=94826) [details]
Add heap export symbols.
------- Comment #152 From 2011-05-25 12:03:47 PST -------
Created an attachment (id=94829) [details]
Add interpreter export symbols.
------- Comment #153 From 2011-05-25 12:07:51 PST -------
Created an attachment (id=94832) [details]
Add parser export symbols.
------- Comment #154 From 2011-05-25 12:32:51 PST -------
Created an attachment (id=94836) [details]
Add profiler export symbols.
------- Comment #155 From 2011-06-08 08:22:08 PST -------
Does anyone have issue with these patches? They've been in review for a while but I haven't seen any comments. If not, would someone be willing to at least quickly rubberstamp these (and the rest that are coming) so that we can move on to getting a port using them and getting them on a build bot? I think most would agree that this method will make life easier for ports, but we need to get it in the tree first. :)

As Eric said, the approach has been approved, and if any of these patches are wrong in some way, it will show up in the build bots. So if the build bots are green, then they should be good to go. I'd really appreciate any help on moving this forward.
------- Comment #156 From 2011-06-08 13:04:41 PST -------
(From update of attachment 94836 [details])
Clearing flags on attachment: 94836

Committed r88376: <http://trac.webkit.org/changeset/88376>
------- Comment #157 From 2011-06-08 17:51:40 PST -------
Created an attachment (id=96518) [details]
Update Heap patch to address bitrot
------- Comment #158 From 2011-06-08 17:59:15 PST -------
Thanks for your help, Eric! The rest of the patches were landed in revisions 88370, 88372, and 88374. (As of 88376, I figured out how to get webkit-patch to do it for me finally. ;-) The Heap one did not apply, and updating the build affected files not in the patch, so I felt I should re-submit. Will have some more up for review shortly! Thanks again!
------- Comment #159 From 2011-06-08 18:09:21 PST -------
Created an attachment (id=96523) [details]
Export macros for JSCell.h
------- Comment #160 From 2011-06-08 18:15:10 PST -------
Created an attachment (id=96524) [details]
Export macros for Identifier.h
------- Comment #161 From 2011-06-08 18:21:44 PST -------
Created an attachment (id=96526) [details]
Export macros for JSGlobalData.h
------- Comment #162 From 2011-07-04 12:19:02 PST -------
Another ping on this. Would someone be willing to rubber-stamp an updated patch with all the symbols in it? It seems that breaking the symbols down into small files isn't helping speed the review up any, and really this isn't the sort of change that can be a source of feature regressions - if there's a problem, compilation will simply break, so the build bot is really doing the needed checks on this. At the current pace, reviews are taking so long that the patches no longer compile by the time the review happens, and it's probably going to take a year+ to land if we continue this way. If someone would just be willing to r+ the symbols patch, we could quickly move on to getting ports using this.
------- Comment #163 From 2011-07-05 06:32:54 PST -------
(In reply to comment #162)
> Another ping on this. Would someone be willing to rubber-stamp an updated patch with all the symbols in it?

I'll rubber-stamp it if it just adds JS_EXPORTDATA macros and the EWS bots like it.
------- Comment #164 From 2011-07-11 09:51:45 PST -------
Created an attachment (id=100313) [details]
Complete set of JS_EXPORT_PRIVATE and WTF_EXPORT_PRIVATE macro additions

Thanks Adam for offering to Rubber Stamp this! A couple notes:

1) The export macros we're using right now are JS_EXPORT_PRIVATE and WTF_EXPORT_PRIVATE. This is so that we can define them to nothing on any port that hasn't moved over yet to ease transition and keep ports that rely on the existing macros from having breakages. Once all the ports are moved over, these macros and JS_EXPORTDATA / JS_EXPORTCLASS will be merged into one macro each for JS and WTF. 

2) There are a few style failures remaining. I can fix these in a subsequent patch, but I didn't change them now because most of them are namespace indentation issues which would cause the patch to show every line in the file as changed, and hence you wouldn't be able to confirm they are all macro additions any longer. The one other remaining style issue is saying I should switch RefPtr to PassRefPtr, but I wasn't sure if that might affect runtime behavior and I felt that would take the patch outside of the scope in which people would be willing to rubber stamp it. 

Thanks again, and sorry this took time, I've been pretty busy recently.
------- Comment #165 From 2011-07-11 09:53:35 PST -------
Attachment 100313 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/heap/HandleHeap.h', ..." exit_code: 1

Source/JavaScriptCore/runtime/JSWrapperObject.h:31:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/wtf/NullPtr.h:45:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/StringObject.h:29:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/wtf/FastMalloc.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/InitializeThreading.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/wtf/RandomNumber.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/UString.h:50:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/JavaScriptCore/wtf/RefCountedLeakCounter.h:29:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 8 in 65 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #166 From 2011-07-11 10:09:15 PST -------
(From update of attachment 100313 [details])
Attachment 100313 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9010524
------- Comment #167 From 2011-07-11 10:18:00 PST -------
(From update of attachment 100313 [details])
Attachment 100313 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9011537
------- Comment #168 From 2011-07-11 10:25:53 PST -------
Okay, the chromium issue is something simple that I've already fixed in my tree (I thought I had fixed it already, actually), but the Qt failure looks new. My guess is that the generated files for WebKitTestRunner are not including any config.h, and hence the JS_EXPORT_PRIVATE and WTF_EXPORT_PRIVATE macros are completely undefined. Can someone on the Qt side offer any suggestions as to the best way to get those files including some form of config.h?

Thanks,

Kevin
------- Comment #169 From 2011-07-11 11:26:27 PST -------
(From update of attachment 100313 [details])
Attachment 100313 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9007732
------- Comment #170 From 2011-07-11 11:43:36 PST -------
(From update of attachment 100313 [details])
Attachment 100313 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9014267
------- Comment #171 From 2011-07-11 12:44:19 PST -------
(From update of attachment 100313 [details])
Attachment 100313 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9006954
------- Comment #172 From 2011-07-11 21:49:42 PST -------
Created an attachment (id=100437) [details]
Fix macro defines for WebKitTestRunner by including config.h

This should fix all the build breakages caused by WTF_EXPORT_PRIVATE and JS_EXPORT_PRIVATE not being defined in WebKitTestRunner generated files.
------- Comment #173 From 2011-07-11 21:58:19 PST -------
(From update of attachment 100437 [details])
Attachment 100437 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9021054
------- Comment #174 From 2011-07-12 09:35:59 PST -------
Created an attachment (id=100500) [details]
New fix for WebKitTestRunner generated files that makes sure config.h is always the first header included.
------- Comment #175 From 2011-07-19 12:15:59 PST -------
Ping on latest patch... problems, questions?
------- Comment #176 From 2011-08-10 16:39:42 PST -------
Created an attachment (id=103555) [details]
Updated complete symbols and config.h fix patch.

Patch against latest trunk containing all symbols and WebKitTestRunner config.h fix. Anyone willing to RS this if it passes the EWS bots? (the style bot will fail with indentation errors but I can fix those before landing.)
------- Comment #177 From 2011-08-10 16:42:02 PST -------
Attachment 103555 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/runtime/JSWrapperObject.h:31:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/wtf/NullPtr.h:47:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/StringObject.h:29:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/wtf/FastMalloc.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/InitializeThreading.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/wtf/RandomNumber.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/UString.h:50:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/JavaScriptCore/wtf/RefCountedLeakCounter.h:29:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 8 in 66 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #178 From 2011-09-03 13:25:14 PST -------
Is there really no one interested in reviewing or rubber-stamping this, or if there's a problem, telling me what is wrong with it? If not, I really feel we should just close this ticket out so that no one continues to put time into something which ultimately will never happen. Which is quite disappointing given the amount of personal time I've put into this. 

I understand everyone is busy, but I would really, really appreciate it if someone would put a little bit of time into reviewing or even just giving me a rs+ to land a patch which does not break the build and which contains only build-time changes, no runtime or behavior changes.
------- Comment #179 From 2011-09-03 14:19:47 PST -------
> Is there really no one interested in reviewing or rubber-stamping this, or if there's a problem, telling me what is wrong with it?

So, I spent some time trying to understand what's going on in this bug.  I suspect the main issue is that you've now got a bug with 170-odd comments and a zillion patches attached.  The patch you're asking to have reviewed is gigantic.

If I were you, I'd close this bug, open a new beta bug (link to this bug) and then start uploading smaller patches in bugs that block the meta bug.

If you've got a reviewer who is interested in seeing this happen, it's much easier to review dozens of small patches that add this macro to an individual file rather than one enormous patch that adds it in a zillion places.

Essentially, this is the strategy that Darin recommend in Comment #57, which I think is good advice.  The way you're approaching things here, it makes it very difficult for folks to help you achieve your goals.

In any case, I'm probably not the right person to help out with this project because I'm not really the right person to be reviewing build configurations changes to JavaScriptCore.
------- Comment #180 From 2011-09-03 15:39:49 PST -------
(In reply to comment #179)
> > Is there really no one interested in reviewing or rubber-stamping this, or if there's a problem, telling me what is wrong with it?
> 
> So, I spent some time trying to understand what's going on in this bug.  I suspect the main issue is that you've now got a bug with 170-odd comments and a zillion patches attached.  The patch you're asking to have reviewed is gigantic.

Yes, but if you actually scroll through it, it is all macro additions, and those are all macros which are defined to nothing in all ports. It's a no-change patch. That comment alone explains pretty much the entire patch.

> If I were you, I'd close this bug, open a new beta bug (link to this bug) and then start uploading smaller patches in bugs that block the meta bug.

I'm willing to do this, or listen to other suggestions as well, but to be honest I want someone to step up to the review plate before I start making changes. I've changed my approach several times already to address comments from devs about what might help get these reviewed, implementing just about every suggestion offered, and so far the reviews have still been few and far between. I actually feel that all the changes I've made to make this patch more 'reviewable' have mostly just confused people and muddied this process.

> If you've got a reviewer who is interested in seeing this happen, it's much easier to review dozens of small patches that add this macro to an individual file rather than one enormous patch that adds it in a zillion places.
>
> Essentially, this is the strategy that Darin recommend in Comment #57, which I think is good advice.  The way you're approaching things here, it makes it very difficult for folks to help you achieve your goals.

Actually, I tried this, and the results were that reviews were (still) few and far between. See #162 and #163 for some context, and #163 particularly should even provide some insight on why I ended up uploading a mega-patch. Someone promised a rubber-stamp on it if it was just macro additions and didn't break the builds... ;-)

> In any case, I'm probably not the right person to help out with this project because I'm not really the right person to be reviewing build configurations changes to JavaScriptCore.

There are no build configuration changes in this patch, though. It is simply adding a bunch of macros, which are defined on all ports as an empty string. I did this because it is what Darin suggested over a year ago, so we can do a 'flip the switch' patch on each port to actually change behavior after all this. This is why it does not break the build. Once this patch is landed, I can file new, much more specific bugs, like "Switch Mac port to getting exported symbols from headers". 

The honest reality of the situation is that 90% of the reviews I've gotten so far on this bug from the start have been RS, including almost every single patch to add macros to the tree. The only significant one that wasn't, IIRC, was the one that r+ed the approach itself, the important one. So rather than spend much of the next year having dozens of patches RSed at a speed of a few a month or so, why not just have one big RS to add all the symbol macros, get the whole thing over with, and you can blame me if everything breaks horribly? Except that it won't, I can guarantee it. The EWS bots guarantee it. Then we can move on to new bugs, new patches, and EVERYTHING submitted will be considerably smaller than this.
------- Comment #181 From 2011-09-03 16:07:21 PST -------
Kevin, I understand that you're frustrated at how long this process has taken.  The reason folks haven't been engaging with you in this discussion is because you've posted a 90k patch on a bug with 180 comments.  It took me an hour to get up to speed enough to even understand what was going on here.  If you want folks to help you out, you'll probably have better luck if you make it easy for them to do what you want.
------- Comment #182 From 2011-09-03 16:56:37 PST -------
(In reply to comment #181)
> Kevin, I understand that you're frustrated at how long this process has taken.  The reason folks haven't been engaging with you in this discussion is because you've posted a 90k patch on a bug with 180 comments.  
>
> It took me an hour to get up to speed enough to even understand what was going on here.  If you want folks to help you out, you'll probably have better luck if you make it easy for them to do what you want.

So if I do what you suggest, can I count on you to help me by doing timely reviews of the dozens of small patches I post in the new bug?
------- Comment #183 From 2011-09-03 17:41:59 PST -------
> So if I do what you suggest, can I count on you to help me by doing timely reviews of the dozens of small patches I post in the new bug?

I'd feel confident reviewing patches in this area if:

1) I was sure that the stakeholders agree that this is a good direction for this code.
2) The patches are manageable in size (e.g., some small number of files per patch).
3) I have some way of knowing that you're adding the macro to the correct functions (e.g., a pointer to the same functions listed in the existing exp file).

From reading this thread, it sounds like you've more or less got (1) under control.  I'd probably want one of the main stakeholders here to review the first patch to confirm that they're still on board.  If you're up for (2) and (3), then it will be easy for me to see that your patches are correct and I'll be able to review them.
------- Comment #184 From 2011-09-03 17:43:37 PST -------
> the dozens of small patches I post in the new bug?

One minor additional point.  Please don't post dozens of patches in a single bug.  It's much easier to keep track of things if you stick to one patch per bug and have the individual bugs block a meta bug.  That keeps any individual bug from "going epic" as this bug has.
------- Comment #185 From 2011-09-03 19:55:16 PST -------
(In reply to comment #183)
> > So if I do what you suggest, can I count on you to help me by doing timely reviews of the dozens of small patches I post in the new bug?
> 
> I'd feel confident reviewing patches in this area if:
> 
> 1) I was sure that the stakeholders agree that this is a good direction for this code.
> 2) The patches are manageable in size (e.g., some small number of files per patch).
> 3) I have some way of knowing that you're adding the macro to the correct functions (e.g., a pointer to the same functions listed in the existing exp file).
> 
> From reading this thread, it sounds like you've more or less got (1) under control.  I'd probably want one of the main stakeholders here to review the first patch to confirm that they're still on board.  If you're up for (2) and (3), then it will be easy for me to see that your patches are correct and I'll be able to review them.

Number 2 is doable, but number 3 is going to balloon the amount of time it takes producing patches as I will probably need to do that by hand. And actually, I'm quite certain #3 is the primary sticking point behind the review problems on this bug.

Plus, we'll largely be duplicating work the compiler and linker does for us anyway. When we do move the ports over to using the symbol macros, the link will fail if the symbol lists are missing a needed symbol - and this will be caught by the EWS bots. If we've missed something, it will be found.

I thought #3 may really be the sticking point for everyone, and that was my reason for suggesting closing this out. There's a point at which this becomes so time-consuming as to no longer justify the effort. I simply don't think there's any real practical need for that level of review on a patch the compiler can vet for us.
------- Comment #186 From 2011-09-03 20:43:30 PST -------
The reason (3) is important is that it is what tells me whether the patch is correct.  Otherwise, I'm left to wonder, for example, why these lines are correct:

-static void writeBarrier(const JSCell*, JSValue);
+JS_EXPORT_PRIVATE static void writeBarrier(const JSCell*, JSValue);
 static void writeBarrier(const JSCell*, JSCell*);

It's not clear to me why writeBarrier should be exported for JSValue but not for JSCell.  I'm sure there's a reason.  It just require more expertise than I have in this code.

The easier you make it for reviewers to see that your patches are "obviously correct" the easier it is for folks to review them.  The more they require thought and knowledge, the less folks will be able to help you.

In any case, you shouldn't feel any obligation to continue with this patch if you're not interested in it anymore.  If someone else wants this to happen, they can certainly pick up where you've left off.
------- Comment #187 From 2011-09-03 21:38:10 PST -------
(In reply to comment #186)
> The reason (3) is important is that it is what tells me whether the patch is correct.  Otherwise, I'm left to wonder, for example, why these lines are correct:
> 
> -static void writeBarrier(const JSCell*, JSValue);
> +JS_EXPORT_PRIVATE static void writeBarrier(const JSCell*, JSValue);
>  static void writeBarrier(const JSCell*, JSCell*);
> 
> It's not clear to me why writeBarrier should be exported for JSValue but not for JSCell.  I'm sure there's a reason.  It just require more expertise than I have in this code.

Although they are the same function, the function prototype with the JS_EXPORT_PRIVATE macro is being called from outside of JSCore, that is, by WebCore (or possibly DRT, etc.) code. The other variant happens not to be, so we leave it as JSCore internal. This is the strategy taken by the entire patch, at the request of Darin and others, because it matches the current approach taken with the exp file lists. 

> The easier you make it for reviewers to see that your patches are "obviously correct" the easier it is for folks to review them.  The more they require thought and knowledge, the less folks will be able to help you.

I don't mind answering questions like the one above. I'm glad someone is asking them, in fact. :) Since the way we export symbols is a project-level policy AFAIU, I just assumed most devs were on board as to how the exports are determined.

However, in terms of seeing it's obviously correct, the only real way to confirm that with 100% certainty is through linking. Of course, if it's wrong, it won't link, so that part of the review is sort of tackled for you. :)

> In any case, you shouldn't feel any obligation to continue with this patch if you're not interested in it anymore.  If someone else wants this to happen, they can certainly pick up where you've left off.

I'm certainly interested in it, very much so. In fact, for me, this is actually a very big deal because the wx port needs the proper export symbol defines to run DRT, so for a while now DRT has been DOA, which I don't need to tell you is not good news.

I have two options to resolve this: 

1) start maintaining wx-port JSCore symbol lists for gcc and MSVC by hand like the other ports do currently (and further add to the maintenance burden of managing export symbols), or 

2) get this bug tackled so that whenever a symbol export is added, the person adding it can make one change and update all ports automatically. 

While #1 was the path of least resistance, I felt #2 was the right way to solve the problem, even if there was more initial up-front investment in the work.

So I really, really, want to see this happen, the sooner the better.
------- Comment #188 From 2011-11-08 23:22:45 PST -------
Hi folks,

I asked Kevin for joining this effort.

To make this happen, I'd like to take an incremental approach.
Here is a plan:

 * Add a macro for Mac port, replacing single export entry with the macro for trial.

 * Replace remaining exported symbols with this macro. 
   For safety, This step will be done by multiple patches.
   I don't think we should remove exp file at this timing. 
   It better be done by some Apple folks because the change can affect the build system.

 * Add macro definitions for each ports and remove related export list entries, one-by-one.

   * Windows, Cairo-Windows and GTK (and some else?)

   * For each list goes empty, file a bug and ask the port maintainers to remove the export/filter/def files.

What do you think? 
I'm going to use this bug for tracking and prepare the first step in a sub-bug of Bug 67852
since this thread is too long to conduct a review.

Regards,
------- Comment #189 From 2011-11-09 00:22:40 PST -------
(In reply to comment #188)

>  * Add a macro for Mac port, replacing single export entry with the macro for trial.
And this step is already done.
What we need is a way to splitting changes on JSC into reviewable small pieces.
------- Comment #190 From 2012-08-01 15:59:54 PST -------
(From update of attachment 103555 [details])
This patch has been up for review for almost a year.  it appears you're missing necessary buy-in from a reviewer.  I don't think this serves much purpose being marked r? still.
------- Comment #191 From 2012-08-01 17:39:51 PST -------
(In reply to comment #190)
> (From update of attachment 103555 [details] [details])
> This patch has been up for review for almost a year.  it appears you're missing necessary buy-in from a reviewer.  I don't think this serves much purpose being marked r? still.
Actually this one is done in one of the sub-bugs. Sorry for leaving this r?.
------- Comment #192 From 2012-09-21 16:51:37 PST -------
This should be closed now that bug 67852 is finished and incorporates the work in this bug.