Bug 27551

Created attachment 33357 [details] Make it possible to build JSC as a shared library without symbol lists

Created attachment 33358 [details] Build JSC as a shared library for QtWebKit

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 on attachment 33277 [details] Patch to make it possible to build JavaScriptCore as shared library on Linux without symbol files. Clearing review after splitting up the patch

Comment on attachment 33357 [details] Make it possible to build JSC as a shared library without symbol lists 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 on attachment 33358 [details] Build JSC as a shared library for QtWebKit Clearing review on this one, too, as it's not useful without the other patch

Created attachment 33626 [details] Updated patch, rebased against ToT and fixed windows and mac builds.

Created attachment 33627 [details] Rebased patch to build JavaScriptCore as shared library with Qt

Comment on attachment 33626 [details] Updated patch, rebased against ToT and fixed windows and mac builds. > * 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.

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

(In reply to comment #10) > - unnecessary JS_EXPORTDATA macros for unions I mean enums of course.

Comment on attachment 33626 [details] Updated patch, rebased against ToT and fixed windows and mac builds. 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.

Created attachment 33773 [details] Updated patch with one MSVC warning fixed and explanation for the other ignored warning

Comment on attachment 33773 [details] Updated patch with one MSVC warning fixed and explanation for the other ignored warning 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.

Mark, can you have a look at these?

Comment on attachment 33627 [details] Rebased patch to build JavaScriptCore as shared library with Qt Should this be marked obsolete? Assuming so.

Comment on attachment 33627 [details] Rebased patch to build JavaScriptCore as shared library with Qt No this one shouldn't be obsolete, but it depends on the other patch. How can I make this relation clear using bugzilla?

Comment on attachment 33773 [details] Updated patch with one MSVC warning fixed and explanation for the other ignored warning 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.

Created attachment 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 on attachment 34737 [details] Updated patch with JS_EXPORTDATA renamed to JS_EXPORT_PRIVATE Clearing review, I forgot two hunks. Oops.

Created attachment 34831 [details] Patch to remove unused WEBKIT_EXPORTDATA macro This patch removes the unused WEBKIT_EXPORTDATA macro.

Created attachment 34832 [details] Rebased patch to build JSC with JS_EXPORT_PRIVATE to export symbols

(In reply to comment #18) > (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. 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.

These are marked for review by Mark. He's a busy guy, you might try tracking him down via #webkit.

Comment on attachment 34832 [details] Rebased patch to build JSC with JS_EXPORT_PRIVATE to export symbols Clearning review as these patches require rebasing by now.

(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?

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

(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?

Created attachment 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.

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.

Attachment 58961 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/3273248

Attachment 58961 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3268239

Attachment 58961 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/3309251

Attachment 58961 [details] did not build on win: Build output: http://webkit-commit-queue.appspot.com/results/3324280

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.

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

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.

Created attachment 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?

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.

Attachment 59038 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/3317325

Created attachment 59052 [details] Same as last patch with proposed GTK build fix

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.

Attachment 59052 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/3326297

Attachment 59052 [details] did not build on win: Build output: http://webkit-commit-queue.appspot.com/results/3328293

Created attachment 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.

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.

Attachment 59298 [details] did not build on win: Build output: http://webkit-commit-queue.appspot.com/results/3336581

Created attachment 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?

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.

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.

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

(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?

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

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

Created attachment 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.

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.

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.

(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.)

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

Attachment 61720 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3557030

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.

I would advice to skip the style parts from the patch for easier review.

Created attachment 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.

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.

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.

Attachment 65843 [details] did not build on mac: Build output: http://queues.webkit.org/results/3884061

Attachment 65843 [details] did not build on qt: Build output: http://queues.webkit.org/results/3839083

Attachment 65843 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3834083

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?

Attachment 65843 [details] did not build on win: Build output: http://queues.webkit.org/results/3854087

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

Created attachment 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. :)

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.

Attachment 65865 [details] did not build on win: Build output: http://queues.webkit.org/results/3822118

Comment on attachment 65865 [details] Style and build fixes to update last patch This patch has been up for review for months and doesn't build on Windows.

Created attachment 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.

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.

Attachment 84895 [details] did not build on qt: Build output: http://queues.webkit.org/results/8101355

Attachment 84895 [details] did not build on gtk: Build output: http://queues.webkit.org/results/8105304

Attachment 84895 [details] did not build on win: Build output: http://queues.webkit.org/results/8105305

Attachment 84895 [details] did not build on win: Build output: http://queues.webkit.org/results/8102386

Created attachment 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.

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.

Attachment 84903 [details] did not build on qt: Build output: http://queues.webkit.org/results/8104367

Attachment 84903 [details] did not build on win: Build output: http://queues.webkit.org/results/8107201

Attachment 84903 [details] did not build on win: Build output: http://queues.webkit.org/results/8107206

Created attachment 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.

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.

Attachment 84910 [details] did not build on qt: Build output: http://queues.webkit.org/results/8103456

Attachment 84910 [details] did not build on win: Build output: http://queues.webkit.org/results/8101427

Attachment 84910 [details] did not build on win: Build output: http://queues.webkit.org/results/8107231

Created attachment 84946 [details] Retry - Step 1 another attempt at Win and Qt build fixes

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.

Attachment 84946 [details] did not build on win: Build output: http://queues.webkit.org/results/8103565

Attachment 84946 [details] did not build on qt: Build output: http://queues.webkit.org/results/8105497

Created attachment 84952 [details] Retry - Step 1 more Win and Qt build fixes, getting closer...

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.

Attachment 84952 [details] did not build on win: Build output: http://queues.webkit.org/results/8107352

Attachment 84952 [details] did not build on qt: Build output: http://queues.webkit.org/results/8107359

Created attachment 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.

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.

Attachment 84962 [details] did not build on win: Build output: http://queues.webkit.org/results/8098825

Created attachment 84975 [details] Retry - Step 1 adding config.h for projects in Tools

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.

Attachment 84962 [details] did not build on qt: Build output: http://queues.webkit.org/results/8103610

Attachment 84975 [details] did not build on win: Build output: http://queues.webkit.org/results/8105563

Attachment 84975 [details] did not build on win: Build output: http://queues.webkit.org/results/8102661

Attachment 84975 [details] did not build on qt: Build output: http://queues.webkit.org/results/8107440

Created attachment 84999 [details] Retry - Step 1 w/ more config.h additions for projects in Tools

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.

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 on attachment 84999 [details] Retry - Step 1 w/ more config.h additions for projects in Tools 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.

(In reply to comment #112) > (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. 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! :)

Step 1 landed in r81135, thanks!

Created attachment 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. :)

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.

Attachment 87006 [details] did not build on win: Build output: http://queues.webkit.org/results/8252400

Attachment 87006 [details] did not build on win: Build output: http://queues.webkit.org/results/8240957

Created attachment 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?

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.

Attachment 87029 [details] did not build on win: Build output: http://queues.webkit.org/results/8255645

Attachment 87029 [details] did not build on win: Build output: http://queues.webkit.org/results/8255689

Created attachment 87051 [details] Step 2, part 1, third try: More Win build fixes.

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 on attachment 87051 [details] Step 2, part 1, third try: More Win build fixes. 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.

Created attachment 87077 [details] Step 2, part 1: this time, just add the macros, don't flip the switch for wx port

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.

Created attachment 87659 [details] Step 2, part 1: same as previous patch, just updated to deal with bit rot

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.

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.

Created attachment 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.

(In reply to comment #131) > 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. First patch landed in r83121, thanks!

Created attachment 88543 [details] Add export macros to wtf/text/WTFString.h

Comment on attachment 88543 [details] Add export macros to wtf/text/WTFString.h 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.

(In reply to comment #134) > (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. 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. :)

(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?

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

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

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

Created attachment 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.

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.

Created attachment 88955 [details] Add export macros to bytecompiler headers.

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 on attachment 88543 [details] Add export macros to wtf/text/WTFString.h 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 on attachment 88955 [details] Add export macros to bytecompiler headers. r=me.

Do we have a buildbot to test this? We'll need one. Otherwise this build will break constantly.

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

WTFString.h patch landed in r87269, thanks!

bytecompiler_exports.patch landed in r87304, thanks!

Created attachment 94823 [details] Add debugger export symbols.

Created attachment 94826 [details] Add heap export symbols.

Created attachment 94829 [details] Add interpreter export symbols.

Created attachment 94832 [details] Add parser export symbols.

Created attachment 94836 [details] Add profiler export symbols.

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 on attachment 94836 [details] Add profiler export symbols. Clearing flags on attachment: 94836 Committed r88376: <http://trac.webkit.org/changeset/88376>

Created attachment 96518 [details] Update Heap patch to address bitrot

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!

Created attachment 96523 [details] Export macros for JSCell.h

Created attachment 96524 [details] Export macros for Identifier.h

Created attachment 96526 [details] Export macros for JSGlobalData.h

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.

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

Created attachment 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.

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 on attachment 100313 [details] Complete set of JS_EXPORT_PRIVATE and WTF_EXPORT_PRIVATE macro additions Attachment 100313 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9010524

Comment on attachment 100313 [details] Complete set of JS_EXPORT_PRIVATE and WTF_EXPORT_PRIVATE macro additions Attachment 100313 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9011537

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 on attachment 100313 [details] Complete set of JS_EXPORT_PRIVATE and WTF_EXPORT_PRIVATE macro additions Attachment 100313 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/9007732

Comment on attachment 100313 [details] Complete set of JS_EXPORT_PRIVATE and WTF_EXPORT_PRIVATE macro additions Attachment 100313 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9014267

Comment on attachment 100313 [details] Complete set of JS_EXPORT_PRIVATE and WTF_EXPORT_PRIVATE macro additions Attachment 100313 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9006954

Created attachment 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 on attachment 100437 [details] Fix macro defines for WebKitTestRunner by including config.h Attachment 100437 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9021054

Created attachment 100500 [details] New fix for WebKitTestRunner generated files that makes sure config.h is always the first header included.

Ping on latest patch... problems, questions?

Created attachment 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.)

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.

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.

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

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

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.

(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?

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

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

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

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.

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

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,

(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 on attachment 103555 [details] Updated complete symbols and config.h fix patch. 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.

(In reply to comment #190) > (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. Actually this one is done in one of the sub-bugs. Sorry for leaving this r?.

This should be closed now that bug 67852 is finished and incorporates the work in this bug.