Bug 109268 - [v8] isolate parameter added to all v8::peristent calls
Summary: [v8] isolate parameter added to all v8::peristent calls
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-08 00:23 PST by Dan Carney
Modified: 2013-02-08 02:18 PST (History)
4 users (show)

See Also:


Attachments
Patch (37.03 KB, patch)
2013-02-08 00:25 PST, Dan Carney
no flags Details | Formatted Diff | Diff
Patch (50.82 KB, patch)
2013-02-08 01:20 PST, Dan Carney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Carney 2013-02-08 00:23:45 PST
[v8] isolate parameter added to all v8::peristent calls
Comment 1 Dan Carney 2013-02-08 00:25:41 PST
Created attachment 187256 [details]
Patch
Comment 2 Dan Carney 2013-02-08 00:26:54 PST
forgot to rebase bindings tests. wiil do that before commit
Comment 3 Kentaro Hara 2013-02-08 00:34:11 PST
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.
Comment 4 Dan Carney 2013-02-08 00:42:16 PST
(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 5 Dan Carney 2013-02-08 00:46:14 PST
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
Comment 6 Kentaro Hara 2013-02-08 00:57:04 PST
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.
Comment 7 Dan Carney 2013-02-08 01:16:12 PST
(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 8 Dan Carney 2013-02-08 01:17:23 PST
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 9 Kentaro Hara 2013-02-08 01:19:07 PST
Comment on attachment 187256 [details]
Patch

Makes sense to me.
Comment 10 Dan Carney 2013-02-08 01:20:33 PST
Created attachment 187264 [details]
Patch
Comment 11 Dan Carney 2013-02-08 01:21:33 PST
Comment on attachment 187264 [details]
Patch

addressed nit. added rebased bindings tests
Comment 12 WebKit Review Bot 2013-02-08 02:18:52 PST
Comment on attachment 187264 [details]
Patch

Clearing flags on attachment: 187264

Committed r142250: <http://trac.webkit.org/changeset/142250>
Comment 13 WebKit Review Bot 2013-02-08 02:18:56 PST
All reviewed patches have been landed.  Closing bug.