After attempting to switch WebWorkers in-process and run tests I discovered a bunch of statics that should be made per-isolate and several cases of V8 API accessing the wrong isolate.
Created attachment 103086 [details] Fixes.
Comment on attachment 103086 [details] Fixes. Attachment 103086 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9310020
Comment on attachment 103086 [details] Fixes. Attachment 103086 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/9323039
Created attachment 103102 [details] Release compilation fixed
Comment on attachment 103102 [details] Release compilation fixed View in context: https://bugs.webkit.org/attachment.cgi?id=103102&action=review Just a few things to clear/clean up. > Source/WebCore/ChangeLog:11 > + (WebCore::V8BindingPerIsolateData::lazyEventListenerToStringTemplate): Why are all of these items being moved? Ideally they would be moved in one change and if any changes were needed, then that would happen in another change. > Source/WebCore/bindings/v8/V8Binding.h:176 > + inline AllowAllocation() Isn't inline redundant here (since you put the function body in the class definition)? > Source/WebCore/bindings/v8/V8Binding.h:183 > + inline ~AllowAllocation() Ditto. > Source/WebCore/bindings/v8/V8Binding.h:199 > + static inline v8::Local<v8::Object> newInstance(v8::Handle<v8::Function>, int argc, v8::Handle<v8::Value> argv[]); The indentation is following Chromium style as opposed to WebKit style. > Source/WebCore/bindings/v8/V8LazyEventListener.cpp:143 > toStringTemplate = v8::Persistent<v8::FunctionTemplate>::New(v8::FunctionTemplate::New(V8LazyEventListenerToString)); I wonder why this isn't part of the method lazyEventListenerToStringTemplate. > Source/WebKit/chromium/ChangeLog:8 > + * src/BoundObject.cpp:Headers Would be nice to explain why this change was needed. (Now I see why bur it is still nice to add the info here to make it quicker for people to understand.)
Comment on attachment 103102 [details] Release compilation fixed View in context: https://bugs.webkit.org/attachment.cgi?id=103102&action=review >> Source/WebCore/ChangeLog:11 > > Why are all of these items being moved? > > Ideally they would be moved in one change and if any changes were needed, then that would happen in another change. You mean why AllowAllocation is moved between headers? This is because of the header dependencies (V8Proxy include V8Utilities include V8Bindings include V8Proxy). Do you insist on splitting class move between headers and change in class into two different patches? I see little reason for that. >> Source/WebCore/bindings/v8/V8Binding.h:176 >> + inline AllowAllocation() > > Isn't inline redundant here (since you put the function body in the class definition)? You are right - I'll remove this. >> Source/WebCore/bindings/v8/V8Binding.h:199 >> + static inline v8::Local<v8::Object> newInstance(v8::Handle<v8::Function>, int argc, v8::Handle<v8::Value> argv[]); > > The indentation is following Chromium style as opposed to WebKit style. Will fix >> Source/WebCore/bindings/v8/V8LazyEventListener.cpp:143 >> toStringTemplate = v8::Persistent<v8::FunctionTemplate>::New(v8::FunctionTemplate::New(V8LazyEventListenerToString)); > > I wonder why this isn't part of the method lazyEventListenerToStringTemplate. V8BindingPerIsolateData does not need to know anything about V8LazyEventListener, other than provide a container for its data. This logic really belongs here. >> Source/WebKit/chromium/ChangeLog:8 >> + * src/BoundObject.cpp:Headers > > Would be nice to explain why this change was needed. (Now I see why bur it is still nice to add the info here to make it quicker for people to understand.) True that) Will add comments.
Created attachment 103263 [details] CR feedback
(In reply to comment #6) > (From update of attachment 103102 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=103102&action=review > > >> Source/WebCore/ChangeLog:11 > > > > > Why are all of these items being moved? > > > > Ideally they would be moved in one change and if any changes were needed, then that would happen in another change. > > You mean why AllowAllocation is moved between headers? This is because of the header dependencies (V8Proxy include V8Utilities include V8Bindings include V8Proxy). > > Do you insist on splitting class move between headers and change in class into two different patches? I see little reason for that. I won't insist. However, doing this all at once makes it much harder to determine what is being moved vs what is being changed but at the moment and later when someone goes thru diffs to understand what was done.
Comment on attachment 103263 [details] CR feedback Clearing flags on attachment: 103263 Committed r92619: <http://trac.webkit.org/changeset/92619>
All reviewed patches have been landed. Closing bug.
(In reply to comment #8) > (In reply to comment #6) > > (From update of attachment 103102 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=103102&action=review > > > > >> Source/WebCore/ChangeLog:11 > > > > > > > > Why are all of these items being moved? > > > > > > Ideally they would be moved in one change and if any changes were needed, then that would happen in another change. > > > > You mean why AllowAllocation is moved between headers? This is because of the header dependencies (V8Proxy include V8Utilities include V8Bindings include V8Proxy). > > > > Do you insist on splitting class move between headers and change in class into two different patches? I see little reason for that. > > I won't insist. However, doing this all at once makes it much harder to determine what is being moved vs what is being changed but at the moment and later when someone goes thru diffs to understand what was done. Point taken.
Patch rolled out due to breaking chromium canary
Created attachment 103370 [details] Fixed field initialization to unbreak chromium canary.
Comment on attachment 103370 [details] Fixed field initialization to unbreak chromium canary. Clearing flags on attachment: 103370 Committed r92694: <http://trac.webkit.org/changeset/92694>