[v8] isolate parameter added to all v8::peristent calls
Created attachment 187256 [details] Patch
forgot to rebase bindings tests. wiil do that before commit
Comment on attachment 187256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187256&action=review > Source/WebCore/bindings/v8/NPV8Object.cpp:95 > + v8NpObject->v8Object.Dispose(v8::Isolate::GetCurrent()); Can we avoid calling Isolate::GetCurrent() by passing an Isolate around? > Source/WebCore/bindings/v8/NPV8Object.cpp:169 > + v8npObject->v8Object = v8::Persistent<v8::Object>::New(v8::Isolate::GetCurrent(), object); Ditto. > Source/WebCore/bindings/v8/ScheduledAction.cpp:57 > + v8::Isolate* isolate = context->GetIsolate(); Nit: context => m_context (just for consistency) > Source/WebCore/bindings/v8/ScopedPersistent.h:46 > + : m_handle(v8::Persistent<T>::New(v8::Isolate::GetCurrent(), handle)) This would be OK. > Source/WebCore/bindings/v8/ScopedPersistent.h:60 > void set(v8::Handle<T> handle) This should receive an Isolate. If call sites are too many, you can fix it in a follow-up patch (please just add FIXME). > Source/WebCore/bindings/v8/ScopedPersistent.h:77 > + m_handle.Dispose(v8::Isolate::GetCurrent()); Ditto. > Source/WebCore/bindings/v8/V8HiddenPropertyName.cpp:76 > + return v8::Persistent<v8::String>::New(v8::Isolate::GetCurrent(), v8::String::NewSymbol(key)); I think this can be fixed.
(In reply to comment #3) > (From update of attachment 187256 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=187256&action=review > > > Source/WebCore/bindings/v8/NPV8Object.cpp:95 > > + v8NpObject->v8Object.Dispose(v8::Isolate::GetCurrent()); > > Can we avoid calling Isolate::GetCurrent() by passing an Isolate around? I'll see if any of these are easily fixable now, but we are going to remove all instances of GetCurrent() in followup patches, so there is no need for 100s or FIXMEs throughout the code.
Comment on attachment 187256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187256&action=review >> Source/WebCore/bindings/v8/NPV8Object.cpp:95 >> + v8NpObject->v8Object.Dispose(v8::Isolate::GetCurrent()); > > Can we avoid calling Isolate::GetCurrent() by passing an Isolate around? All np stuff is on the main isolate. I'm going to add a static variable for the main world isolate, which is why I don't want to pass parameters around here. Again, no need for zillions of fixmes here either
Understood. No FIXME is OK. But I'm not quite happy with a lot of Isolate::GetCurrent()s with "let's fix it later". Alternately, you can first land patches that pass an Isolate around so that you don't need to write Isolate::GetCurrent() too much.
(In reply to comment #6) > Understood. No FIXME is OK. But I'm not quite happy with a lot of Isolate::GetCurrent()s with "let's fix it later". Alternately, you can first land patches that pass an Isolate around so that you don't need to write Isolate::GetCurrent() too much. We'd prefer a mechanical replacement of all calls with a Isolate::GetCurrent() unless an isolate parameter is already available. After all calls are replaced, then it will be easier to analyze which calls need an isolate parameter, and which classes need an isolate member added. Doing it in stages might not be optimal and will slow down this work. Adding Isolate::GetCurrent() adds no overhead, and we WILL remove all calls to GetCurrent() in the future, since it will no longer do what it used to do once the v8 api is changed.
Comment on attachment 187256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187256&action=review >> Source/WebCore/bindings/v8/ScopedPersistent.h:60 >> void set(v8::Handle<T> handle) > > This should receive an Isolate. If call sites are too many, you can fix it in a follow-up patch (please just add FIXME). I checked. Too many calls site for this patch. >> Source/WebCore/bindings/v8/V8HiddenPropertyName.cpp:76 >> + return v8::Persistent<v8::String>::New(v8::Isolate::GetCurrent(), v8::String::NewSymbol(key)); > > I think this can be fixed. Too many call sites for this patch.
Comment on attachment 187256 [details] Patch Makes sense to me.
Created attachment 187264 [details] Patch
Comment on attachment 187264 [details] Patch addressed nit. added rebased bindings tests
Comment on attachment 187264 [details] Patch Clearing flags on attachment: 187264 Committed r142250: <http://trac.webkit.org/changeset/142250>
All reviewed patches have been landed. Closing bug.