Bug 84161 – [V8] Add an optional Isolate argument to toV8()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kentaro Hara]

The final objective is to pass Isolate around in V8 bindings. In this bug, we add an optional Isolate argument to toV8(). After rewriting all toV8() callers so that they pass Isolate, I will make the Isolate argument non-optional.

[Kentaro Hara]

Created attachment 137558 [details]
Patch

[WebKit Review Bot]

Attachment 137558 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/bindings/scripts/test/V8/V8Float64Array.h:61:  The parameter name "isolate" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 51 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nate Chapin]

Comment on attachment 137558 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137558&action=review

Will happily r+ once you've got a version that passes EWS.

> Source/WebCore/bindings/v8/custom/V8CSSRuleCustom.cpp:47
> -v8::Handle<v8::Value> toV8(CSSRule* impl)
> +v8::Handle<v8::Value> toV8(CSSRule* impl, v8::Isolate* isolate)
>  {

I assume this is going to compile-fail, since isolate appears to be unused.

[Kentaro Hara]

(In reply to comment #3)
> > Source/WebCore/bindings/v8/custom/V8CSSRuleCustom.cpp:47
> > -v8::Handle<v8::Value> toV8(CSSRule* impl)
> > +v8::Handle<v8::Value> toV8(CSSRule* impl, v8::Isolate* isolate)
> >  {
> 
> I assume this is going to compile-fail, since isolate appears to be unused.

I confirmed that the compile passes in my local Linux. It seems that an unused argument is not an issue in V8 bindings. There are already unused arguments in the generated code.

[Nate Chapin]

(In reply to comment #4)
> (In reply to comment #3)
> > > Source/WebCore/bindings/v8/custom/V8CSSRuleCustom.cpp:47
> > > -v8::Handle<v8::Value> toV8(CSSRule* impl)
> > > +v8::Handle<v8::Value> toV8(CSSRule* impl, v8::Isolate* isolate)
> > >  {
> > 
> > I assume this is going to compile-fail, since isolate appears to be unused.
> 
> I confirmed that the compile passes in my local Linux. It seems that an unused argument is not an issue in V8 bindings. There are already unused arguments in the generated code.

Interesting, I didn't realize we had unused variables kicking around the bindings. I thought mac compiles broke on unused variables much more aggressively.

[Kentaro Hara]

(In reply to comment #5)
> Interesting, I didn't realize we had unused variables kicking around the bindings. I thought mac compiles broke on unused variables much more aggressively.

Yes, in mac it seems that unused variables are not allowed.

e.g. The generated code for V8 binding. 'name' is not used:

static v8::Handle<v8::Value> idAttrGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info)
{                                                                                       
    INC_STATS("DOM.HTMLElement.id._get");
    return getElementStringAttr(info, HTMLNames::idAttr);
}

Anyway I'll keep watching ews bots. Let me ping you if it becomes green:)

[Nate Chapin]

Comment on attachment 137558 [details]
Patch

Alright then, LGTM. Please fix the style warning before landing.

[Kentaro Hara]

(In reply to comment #7)
> (From update of attachment 137558 [details])
> Alright then, LGTM. Please fix the style warning before landing.

I think the warning is false positive. The 'isolate' is needed to set the default value in the header file.

[Kentaro Hara]

Committed r114401: <http://trac.webkit.org/changeset/114401>

[Kentaro Hara]

Reverted r114401 for reason:

Chromium crash

Committed r114732: <http://trac.webkit.org/changeset/114732>

[Kentaro Hara]

Created attachment 138362 [details]
Patch for relanding. It is revealed that the patch was innocent.

[Kentaro Hara]

Committed r114907: <http://trac.webkit.org/changeset/114907>