Bug 84161 - [V8] Add an optional Isolate argument to toV8()
Summary: [V8] Add an optional Isolate argument to toV8()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks: 84074
  Show dependency treegraph
 
Reported: 2012-04-17 10:23 PDT by Kentaro Hara
Modified: 2012-04-23 09:20 PDT (History)
6 users (show)

See Also:


Attachments
Patch (44.61 KB, patch)
2012-04-17 10:33 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch for relanding. It is revealed that the patch was innocent. (44.66 KB, patch)
2012-04-23 09:19 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2012-04-17 10:23:19 PDT
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.
Comment 1 Kentaro Hara 2012-04-17 10:33:57 PDT
Created attachment 137558 [details]
Patch
Comment 2 WebKit Review Bot 2012-04-17 10:37:37 PDT
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.
Comment 3 Nate Chapin 2012-04-17 10:43:07 PDT
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.
Comment 4 Kentaro Hara 2012-04-17 10:47:06 PDT
(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.
Comment 5 Nate Chapin 2012-04-17 10:50:02 PDT
(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.
Comment 6 Kentaro Hara 2012-04-17 10:53:24 PDT
(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:)
Comment 7 Nate Chapin 2012-04-17 11:16:11 PDT
Comment on attachment 137558 [details]
Patch

Alright then, LGTM. Please fix the style warning before landing.
Comment 8 Kentaro Hara 2012-04-17 11:19:27 PDT
(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.
Comment 9 Kentaro Hara 2012-04-17 11:21:50 PDT
Committed r114401: <http://trac.webkit.org/changeset/114401>
Comment 10 Kentaro Hara 2012-04-20 02:31:09 PDT
Reverted r114401 for reason:

Chromium crash

Committed r114732: <http://trac.webkit.org/changeset/114732>
Comment 11 Kentaro Hara 2012-04-23 09:19:45 PDT
Created attachment 138362 [details]
Patch for relanding. It is revealed that the patch was innocent.
Comment 12 Kentaro Hara 2012-04-23 09:20:28 PDT
Committed r114907: <http://trac.webkit.org/changeset/114907>