Summary: | [V8] Add an optional Isolate argument to toV8() | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kentaro Hara <haraken> | ||||||
Component: | WebCore JavaScript | Assignee: | Kentaro Hara <haraken> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, dglazkov, eric.carlson, feature-media-reviews, japhet, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 84074 | ||||||||
Attachments: |
|
Description
Kentaro Hara
2012-04-17 10:23:19 PDT
Created attachment 137558 [details]
Patch
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 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. (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. (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. (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 on attachment 137558 [details]
Patch
Alright then, LGTM. Please fix the style warning before landing.
(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. Committed r114401: <http://trac.webkit.org/changeset/114401> Reverted r114401 for reason: Chromium crash Committed r114732: <http://trac.webkit.org/changeset/114732> Created attachment 138362 [details]
Patch for relanding. It is revealed that the patch was innocent.
Committed r114907: <http://trac.webkit.org/changeset/114907> |