Convert markstack to a slot visitor API
Created attachment 80382 [details] Patch
Attachment 80382 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObject..." exit_code: 1 Source/JavaScriptCore/runtime/JSArray.h:30: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 114 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 80382 [details] did not build on qt: Build output: http://queues.webkit.org/results/7651064
Attachment 80382 [details] did not build on win: Build output: http://queues.webkit.org/results/7593365
Created attachment 80390 [details] Patch
Attachment 80390 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObject..." exit_code: 1 Source/JavaScriptCore/runtime/JSArray.h:30: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 114 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 80390 [details] did not build on qt: Build output: http://queues.webkit.org/results/7532385
Attachment 80390 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7561392
Attachment 80390 [details] did not build on mac: Build output: http://queues.webkit.org/results/7603393
- ALWAYS_INLINE void append(JSValue); - void append(JSCell*); + void append(JSValue*); + void append(JSCell**); I think we want these APIs to be deprecatedAppend(JSValue*); template <typename T> deprecatedAppend(DeprecatedPtr<T>); template <typename T> append(WriteBarrier<T>); We rely on MarkStack's interface, and the help of the compiler, to suss out all the objects that aren't yet moving-safe. I think it's worth it to limit the surface area here, not allowing any JSCell**, and to call out the deprecated interfaces explicitly.
+ template <> class WriteBarrierBase<Unknown> { What's the goal of this separate base class? + protected: + EncodedJSValue m_value; Can we store a JSValue instead of an EncodedJSValue? +++ b/Source/JavaScriptCore/runtime/JSValue.h @@ -23,6 +23,8 @@ #ifndef JSValue_h #define JSValue_h +#include "WriteBarrier.h" It's confusing that half the WriteBarrier and DeprecatedPtr class is in WriteBarrier.h, and half is in JSValue.h. Can we put them in the same file, and have WriterBarrier.h #include JSValue.h? JSValue declares two constructor declarations that depend on a template declaration, but I think you can just add that those declarations stand-alone.
(In reply to comment #11) > + template <> class WriteBarrierBase<Unknown> { > > What's the goal of this separate base class? > > + protected: > + EncodedJSValue m_value; > > Can we store a JSValue instead of an EncodedJSValue? Nope. > > > +++ b/Source/JavaScriptCore/runtime/JSValue.h > @@ -23,6 +23,8 @@ > #ifndef JSValue_h > #define JSValue_h > > +#include "WriteBarrier.h" > > It's confusing that half the WriteBarrier and DeprecatedPtr class is in WriteBarrier.h, and half is in JSValue.h. Can we put them in the same file, and have WriterBarrier.h #include JSValue.h? JSValue declares two constructor declarations that depend on a template declaration, but I think you can just add that those declarations stand-alone. I will give that a shot
> > Can we store a JSValue instead of an EncodedJSValue? > Nope. Can you explain why we need to store an EncodedJSValue?
(In reply to comment #13) > > > Can we store a JSValue instead of an EncodedJSValue? > > Nope. > > Can you explain why we need to store an EncodedJSValue? Because using a JSValue creates an implicit constructor in WriteBarrierBase. We need WriteBarrierBase to have no constructor so that we can put it in the JSObject data union. That's also why there's the WriteBarrierBase and WriteBarrier distinction - WriteBarrier is the easier one to write, and has helpful constructors, but we need a common base class so that JSObject is sane.
Created attachment 80482 [details] Patch
Attachment 80482 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObject..." exit_code: 1 Source/JavaScriptCore/runtime/JSArray.h:30: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 121 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 80482 [details] did not build on qt: Build output: http://queues.webkit.org/results/7552349
Created attachment 80503 [details] Patch
Attachment 80503 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObject..." exit_code: 1 Source/JavaScriptCore/runtime/JSArray.h:30: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 124 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 80503 [details] Patch r=me
Committed r77006: <http://trac.webkit.org/changeset/77006>
This patch broke hundreds of layout tests on Windows 7: http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r77004%20(8776)/results.html http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r77006%20(8777)/results.html I'm sorry but I had to roll it out in http://trac.webkit.org/changeset/77044.
(In reply to comment #22) > This patch broke hundreds of layout tests on Windows 7: > http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r77004%20(8776)/results.html > http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r77006%20(8777)/results.html > > I'm sorry but I had to roll it out in http://trac.webkit.org/changeset/77044. Rolled back in with 32bit fix in r77098
Reopen, because it was rolled out by http://trac.webkit.org/changeset/77113 Qt build was broken for ~6 hours and 7 patches are stucked in Qt EWS because of this build break. Qt EWS and Qt folks always willingly help you to fix the Qt build, but breaking the build for hours isn't a fairplay game ... Please respect and don't ignore "Keeping the tree green" policy: http://webkit.org/coding/contributing.html "Your change must at least compile on all platforms."
Comment on attachment 80503 [details] Patch remove r+ from landed patch
(In reply to comment #24) > Reopen, because it was rolled out by http://trac.webkit.org/changeset/77113 > > Qt build was broken for ~6 hours and 7 patches are stucked in Qt EWS because of this build break. > > Qt EWS and Qt folks always willingly help you to fix the Qt > build, but breaking the build for hours isn't a fairplay game ... > > Please respect and don't ignore "Keeping the tree green" policy: > http://webkit.org/coding/contributing.html > "Your change must at least compile on all platforms." I emailed simon for assistance, there is no clear goto person for qt js bindings. The build failures should have been trivial for someone to fix if they had a qt build. Also afaict the build bot output was giving invalid line numbers. There's bindings code for qt that is not using normal webcore hashmaps so my ability to actually fix code that's using them is severely limited. I will land this again with my approach to "working build" be compiling. Even if the code is obviously incorrect. Personally I would have tried being more polite to someone who actually put effort into fixing things.
(In reply to comment #24) > Reopen, because it was rolled out by http://trac.webkit.org/changeset/77113 > > Qt build was broken for ~6 hours and 7 patches are stucked in Qt EWS because of this build break. > > Qt EWS and Qt folks always willingly help you to fix the Qt > build, but breaking the build for hours isn't a fairplay game ... > > Please respect and don't ignore "Keeping the tree green" policy: > http://webkit.org/coding/contributing.html > "Your change must at least compile on all platforms." Sorry i forgot: I also told those qt people that appeared to be on irc when i logged off about what they needed to do to fix the build. Clearly I'm a bastard.