Bug 84202 - [V8] Add an optional Isolate argument to wrap()
Summary: [V8] Add an optional Isolate argument to wrap()
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 15:32 PDT by Kentaro Hara
Modified: 2012-04-23 09:51 PDT (History)
6 users (show)

See Also:


Attachments
Patch (25.56 KB, patch)
2012-04-17 15:36 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch for relanding. It is revealed that the patch was innocent. (26.13 KB, patch)
2012-04-23 09:50 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 15:32:28 PDT
The final objective is to pass Isolate around in V8 bindings. In this bug, we add an optional Isolate argument to wrap(). After rewriting all wrap() callers so that they pass Isolate to wrap(), I'll make the Isolate argument non-optional.
Comment 1 Kentaro Hara 2012-04-17 15:36:54 PDT
Created attachment 137620 [details]
Patch
Comment 2 WebKit Review Bot 2012-04-17 15:39:25 PDT
Attachment 137620 [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/V8TestEventTarget.h:43:  The parameter name "isolate" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/scripts/test/V8/V8TestEventConstructor.h:44:  The parameter name "isolate" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/scripts/test/V8/V8TestInterface.h:45:  The parameter name "isolate" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/scripts/test/V8/V8TestNode.h:43:  The parameter name "isolate" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/scripts/test/V8/V8Float64Array.h:44:  The parameter name "isolate" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/scripts/test/V8/V8TestObj.h:43:  The parameter name "isolate" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/scripts/test/V8/V8TestNamedConstructor.h:49:  The parameter name "isolate" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/scripts/test/V8/V8TestMediaQueryListListener.h:43:  The parameter name "isolate" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/scripts/test/V8/V8TestActiveDOMObject.h:43:  The parameter name "isolate" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/scripts/test/V8/V8TestCustomNamedGetter.h:43:  The parameter name "isolate" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.h:45:  The parameter name "isolate" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 11 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Kentaro Hara 2012-04-18 08:48:57 PDT
All bots got green. The style error seems false positive (We want to write the default value in the header). Review?
Comment 4 Nate Chapin 2012-04-18 09:29:53 PDT
Comment on attachment 137620 [details]
Patch

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

>> Source/WebCore/bindings/scripts/test/V8/V8Float64Array.h:44
>> +    inline static v8::Handle<v8::Object> wrap(Float64Array*, v8::Isolate* isolate = 0);
> 
> The parameter name "isolate" adds no information, so it should be removed.  [readability/parameter_name] [5]

It's objecting to the variable name, not the assignment. I believe you can change this to "v8::Isolate* = 0"?
Comment 5 Kentaro Hara 2012-04-18 09:54:10 PDT
Committed r114519: <http://trac.webkit.org/changeset/114519>
Comment 6 Kentaro Hara 2012-04-18 09:54:56 PDT
(In reply to comment #4)
> It's objecting to the variable name, not the assignment. I believe you can change this to "v8::Isolate* = 0"?

Fixed. Thanks for reviews!
Comment 7 Kentaro Hara 2012-04-20 02:28:38 PDT
Reverted r114519 for reason:

Chromium crash

Committed r114730: <http://trac.webkit.org/changeset/114730>
Comment 8 Kentaro Hara 2012-04-23 09:50:30 PDT
Created attachment 138367 [details]
Patch for relanding. It is revealed that the patch was innocent.
Comment 9 Kentaro Hara 2012-04-23 09:51:08 PDT
Committed r114911: <http://trac.webkit.org/changeset/114911>