Summary: | Add CodeGenerator support for sequence<> in callbacks | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Rafael Weinstein <rafaelw> | ||||||||||||||||||||||||||||
Component: | WebKit Misc. | Assignee: | Vineet Chaudhary (vineetc) <code.vineet> | ||||||||||||||||||||||||||||
Status: | REOPENED --- | ||||||||||||||||||||||||||||||
Severity: | Normal | CC: | abarth, adamk, arv, code.vineet, dglazkov, haraken, japhet, rafaelw, rakuco, webkit.review.bot | ||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||
Bug Depends on: | 83373, 84232 | ||||||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||||||
Attachments: |
|
Description
Rafael Weinstein
2012-04-04 16:26:46 PDT
Created attachment 135884 [details]
proposed patch
Attaching proposed patch, to get initial feedback.
I will upload patch soon with adding test in TestObj.idl.
Comment on attachment 135884 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=135884&action=review > Source/WebCore/ChangeLog:8 > + No new tests. Would you add a test case to run-bindings-tests, so that we can see what change is going to be made in the generated code? > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2610 > + my $argType = $codeGenerator->GetArrayType($param->type); $arrayType would be a better name than $argType. Same comments for other $argType-s. Created attachment 135890 [details]
patch_for_review
Comment on attachment 135884 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=135884&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2697 > + if ($argType eq "MutationRecordArray") { > + AddIncludesForTypeInImpl("MutationRecord", 1); The idl file should just be changed to sequence<MutationRecord> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:147 > + if ($type eq "MutationRecordArray") { same > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3077 > + push(@implContent, " ASSERT(${paramName});\n"); Can you just use? push(@headerContent, <<END); ... END > Source/WebCore/dom/MutationCallback.idl:36 > + boolean handleEvent(in sequence<MutationRecordArray> mutations, in WebKitMutationObserver observer); This should be sequence<MutationRecord> Attachment 135890 [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/JS/JSTestCallback.h:50: The parameter name "observer" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/bindings/scripts/test/V8/V8TestCallback.h:55: The parameter name "observer" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 2 in 15 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 135890 [details] patch_for_review View in context: https://bugs.webkit.org/attachment.cgi?id=135890&action=review > Source/WebCore/bindings/scripts/test/V8/V8TestInterface.cpp:232 > + V8DOMWrapper::setJSWrapperForActiveDOMObject(impl.release(), v8::Persistent<v8::Object>::New(wrapper)); I am not sure if these are related to this patch. I think we rebaseline Test results. Comment on attachment 135890 [details] patch_for_review View in context: https://bugs.webkit.org/attachment.cgi?id=135890&action=review >> Source/WebCore/bindings/scripts/test/V8/V8TestInterface.cpp:232 >> + V8DOMWrapper::setJSWrapperForActiveDOMObject(impl.release(), v8::Persistent<v8::Object>::New(wrapper)); > > I am not sure if these are related to this patch. I think we rebaseline Test results. Sorry, this is me. Rebaselining now. Comment on attachment 135890 [details] patch_for_review View in context: https://bugs.webkit.org/attachment.cgi?id=135890&action=review > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:162 > + MarkedArgumentBuffer list; Can't you use a JSArray directly instead of going through a MarkedArgumentBuffer? > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:167 > + args.append(constructArray(exec, m_data->globalObject(), list)); Yeah, this looks strange. > Source/WebCore/bindings/scripts/test/V8/V8TestCallback.cpp:215 > + mutationsArray->Set(v8::Integer::New(i), toV8(mutations->at(i).get())); v8::Uint32::New(i) seems a bit better Oke I will upload patch with all review commnts. Thanks :) Comment on attachment 135884 [details] proposed patch Attachment 135884 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12336518 New failing tests: fast/mutation/callback-arguments.html Created attachment 135897 [details]
Archive of layout-test-results from ec2-cr-linux-04
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 135890 [details] patch_for_review Attachment 135890 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12340498 New failing tests: fast/mutation/callback-arguments.html Created attachment 135906 [details]
Archive of layout-test-results from ec2-cr-linux-02
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 135884 [details] proposed patch Attachment 135884 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12340509 New failing tests: fast/mutation/callback-arguments.html Created attachment 135912 [details]
Archive of layout-test-results from ec2-cr-linux-04
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 135974 [details] updated_patch > > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:162 > > + MarkedArgumentBuffer list; > > Can't you use a JSArray directly instead of going through a MarkedArgumentBuffer? > > > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:167 > > + args.append(constructArray(exec, m_data->globalObject(), list)); > > Yeah, this looks strange. I checked the implementation of constructArray() If we use JSArray I think we have to repeate code of constructArray() so I thought to keep the current Implemenatation. Please correct me If this isn't way. I have updated patch with all other review comments and fixing the failing test case fast/mutation/callback-arguments.html. The reason for faillure of fast/mutation/callback-arguments.html was As per the WebIDL specification MutationObserver should pass "this" as second argument. Ref: https://bugs.webkit.org/show_bug.cgi?id=81712 and http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#concept-mo-invoke Attachment 135974 [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/JS/JSTestCallback.h:50: The parameter name "observer" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/bindings/scripts/test/V8/V8TestCallback.h:55: The parameter name "observer" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 2 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 135974 [details] updated_patch Attachment 135974 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12264673 Comment on attachment 135974 [details] updated_patch View in context: https://bugs.webkit.org/attachment.cgi?id=135974&action=review > Source/WebCore/bindings/scripts/CodeGenerator.pm:463 > + foreach my $param (@params) { > + return 1 if $param && $generator->GetArrayType($param->type) eq "MutationRecord"; > + } Do you need to hard-code "MutationRecord" here? Maybe the check could be... return 0 if !$dataNode->extendedAttributes{"Callback"}; foreach my $param (@params) { return 1 if $generator->GetArrayType($param->type); # If the interface has [Callback] and one of the method arguments is of type Array. } return 0; > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2728 > + Nit: This new line would not be necessary. > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:153 > +{ We want to insert 'if (!mutations)' check here, just like V8TestCallback.cpp. > Source/WebCore/bindings/scripts/test/V8/V8TestCallback.cpp:212 > + ASSERT(mutations); > + if (!mutations) > + return true; Nit: Shall we move this check to the top of this method? Created attachment 136012 [details]
patch
Attachment 136012 [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/JS/JSTestCallback.h:50: The parameter name "observer" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/bindings/scripts/test/V8/V8TestCallback.h:55: The parameter name "observer" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 2 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 136012 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=136012&action=review > Source/WebCore/bindings/js/JSMutationCallbackCustom.cpp:45 > +/* Remove the function. > Source/WebCore/bindings/scripts/CodeGenerator.pm:457 > +sub SendCallbackThisValue Nit: Maybe we want more descriptive method name. IsCallbackWithArrayType()? > Source/WebCore/bindings/scripts/CodeGenerator.pm:465 > + Nit: remove this line. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2715 > + push(@implContent, "@argsCheck\n") if scalar(@argsCheck) > 0; Nit: Just 'if @argsCheck;' would work. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3069 > + push(@implContent, "@argsCheck\n") if scalar(@argsCheck) > 0; Ditto. > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:174 > + bool raisedException = false; Nit: we normally use 'ec' for the variable name. > Source/WebCore/bindings/v8/custom/V8MutationCallbackCustom.cpp:49 > +/* Remove the function. (In reply to comment #22) > > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:174 > > + bool raisedException = false; > > Nit: we normally use 'ec' for the variable name. Please ignore this comment. This is not an exception code. 'raisedException' is fine. Created attachment 136025 [details] patch_for_landing > > Source/WebCore/bindings/js/JSMutationCallbackCustom.cpp:45 > > Source/WebCore/bindings/v8/custom/V8MutationCallbackCustom.cpp:49 > Remove the function. Done. > > Source/WebCore/bindings/scripts/CodeGenerator.pm:457 > > +sub SendCallbackThisValue > Nit: Maybe we want more descriptive method name. IsCallbackWithArrayType()? Done > > Source/WebCore/bindings/scripts/CodeGenerator.pm:465 > > + > > Nit: remove this line. Done > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2715 > > + push(@implContent, "@argsCheck\n") if scalar(@argsCheck) > 0; > > Nit: Just 'if @argsCheck;' would work. > > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:174 Done Comment on attachment 136025 [details]
patch_for_landing
Perfect!
Attachment 136025 [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/JS/JSTestCallback.h:50: The parameter name "observer" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/bindings/scripts/test/V8/V8TestCallback.h:55: The parameter name "observer" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 2 in 10 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 136025 [details] patch_for_landing Clearing flags on attachment: 136025 Committed r113442: <http://trac.webkit.org/changeset/113442> All reviewed patches have been landed. Closing bug. Comment on attachment 136012 [details]
patch
Landed manually to avoid style check errors.
Comment on attachment 136025 [details] patch_for_landing View in context: https://bugs.webkit.org/attachment.cgi?id=136025&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2746 > + JSValue jsObserver = toJS(exec, m_data->globalObject(), observer); > + m_data->invokeCallback(jsObserver, args, &raisedException); observer is a bit too specific. The argument represents the 'this' object. Now that I think of this more, this looks like a MutationObserver specific hack. Can this be more generic? Right now this will fail if there is another callback that does not have an observer parameter. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3081 > + my @GenerateEventListenerImpl = (); This is not used anywhere > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3110 > + push(@implContent, " return !invokeCallback(m_callback, v8::Handle<v8::Object>::Cast(observerHandle), " . scalar(@params) . ", argv, callbackReturnValue, scriptExecutionContext());\n"); Same here: observerHandle cannot be correct in the general case. > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:176 > + JSValue jsObserver = toJS(exec, m_data->globalObject(), observer); > + m_data->invokeCallback(jsObserver, args, &raisedException); This part need to come from the idl somehow. Adam (K), how is the this-binding explained in the idl? This was reverted in http://trac.webkit.org/changeset/113448 Comment on attachment 136025 [details] patch_for_landing Attachment 136025 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12358255 Created attachment 136206 [details] Patch (In reply to comment #30) > (From update of attachment 136025 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=136025&action=review > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2746 > > + JSValue jsObserver = toJS(exec, m_data->globalObject(), observer); > > + m_data->invokeCallback(jsObserver, args, &raisedException); > > observer is a bit too specific. The argument represents the 'this' object. > > Now that I think of this more, this looks like a MutationObserver specific hack. Can this be more generic? Right now this will fail if there is another callback that does not have an observer parameter. > > > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3081 > > + my @GenerateEventListenerImpl = (); > > This is not used anywhere > > > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3110 > > + push(@implContent, " return !invokeCallback(m_callback, v8::Handle<v8::Object>::Cast(observerHandle), " . scalar(@params) . ", argv, callbackReturnValue, scriptExecutionContext());\n"); > > Same here: observerHandle cannot be correct in the general case. I tried to using $paramName instead. > > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:176 > > + JSValue jsObserver = toJS(exec, m_data->globalObject(), observer); > > + m_data->invokeCallback(jsObserver, args, &raisedException); > > This part need to come from the idl somehow. Please let me know if there is some way to get this from idl. // I am not sure how to do this but Is something below we want? [PassThis] boolean handleEvent(in sequence<MutationRecord> mutations, in WebKitMutationObserver observer); Or we can investigate this with other callbacks and fix then. Also fixing windows build. //Needs removal from JSBindingsAllInOne.cpp Attachment 136206 [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/JS/JSTestCallback.h:50: The parameter name "observer" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/bindings/scripts/test/V8/V8TestCallback.h:55: The parameter name "observer" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 2 in 10 files
If any of these errors are false positives, please file a bug against check-webkit-style.
> > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:176
> > + JSValue jsObserver = toJS(exec, m_data->globalObject(), observer);
> > + m_data->invokeCallback(jsObserver, args, &raisedException);
>
> This part need to come from the idl somehow.
>
Please let me know if there is some way to get this from idl.
// I am not sure how to do this but Is something like below we want?
[PassThis] boolean handleEvent(in sequence<MutationRecord> mutations, in WebKitMutationObserver observer);
Or we can investigate this with other callbacks and fix then.
Can you please provide your feedback on new patch?
(In reply to comment #36) > > > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:176 > > > + JSValue jsObserver = toJS(exec, m_data->globalObject(), observer); > > > + m_data->invokeCallback(jsObserver, args, &raisedException); > > > > This part need to come from the idl somehow. > > > Please let me know if there is some way to get this from idl. > // I am not sure how to do this but Is something like below we want? > [PassThis] boolean handleEvent(in sequence<MutationRecord> mutations, in WebKitMutationObserver observer); > > Or we can investigate this with other callbacks and fix then. > > Can you please provide your feedback on new patch? The WebIDL spec does not provide a way to annotate the 'this' value. By default it is null: http://dev.w3.org/2006/webapi/WebIDL/#es-invoking-callback-functions Looking at the spec of MutationCallback, http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#mutation-observers, I really don't see a way to do this in IDL. It seems like this callback is stuck in custom bindings land. Raf, Adam, do you guys have any ideas? > Raf, Adam, do you guys have any ideas?
We shouldn't feel constrained by WebIDL. It's fine for us to invent new attributes in WebKitIDL. I haven't looked at the details here, but you could imagine inventing one here.
(In reply to comment #38) > > Raf, Adam, do you guys have any ideas? > > We shouldn't feel constrained by WebIDL. It's fine for us to invent new attributes in WebKitIDL. I haven't looked at the details here, but you could imagine inventing one here. Sure, that was my intention. I still don't see how this can be expressed any other way but prose. (In reply to comment #39) > (In reply to comment #38) > > > Raf, Adam, do you guys have any ideas? > > > > We shouldn't feel constrained by WebIDL. It's fine for us to invent new attributes in WebKitIDL. I haven't looked at the details here, but you could imagine inventing one here. > > Sure, that was my intention. I still don't see how this can be expressed any other way but prose. Thanks Adam. Erik, So if I am not getting this wrong, We should introduce new idl attribute to direct the codegenerator. Something like [PassThis] boolean handleEvent(); Created attachment 137291 [details]
Patch
Comment on attachment 137291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137291&action=review Sorry, the MutationCallback is not the cleanest case to start with. I understand why it has lead you to the current set of patches. The thing that makes MutationCallback strange is that it uses the same object as 'this' and the second argument. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2749 > + if ($function->signature->extendedAttributes->{"PassThis"}) { > + foreach my $param (@params) { > + next if $codeGenerator->GetArrayType($param->type); > + my $paramName = $param->name; > + push(@implContent, <<END); > + JSValue js${paramName} = toJS(exec, m_data->globalObject(), ${paramName}); > + m_data->invokeCallback(js${paramName}, args, &raisedException); This is pretty strange. It seems like we treat the first non ArrayType parameter as 'this'. Also, if there are more than 2 parameters we will invoke the callback multiple times. I think we need to always treat the first argument as this when PassThis is set. That would probably mean we also have to include the type [PassThis=WebKitMutationObserver] boolean handleEvent(in sequence<MutationRecord> mutations, in WebKitMutationObserver observer); That handleEvent above happens to pass the observer both as this and as a parameter is an exception, not the norm. > Source/WebCore/bindings/scripts/IDLAttributes.txt:94 > +PassThis Please remember to document this on the wiki once landed http://trac.webkit.org/wiki/WebKitIDL > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:153 > +bool JSTestCallback::callbackRequiresThisToPass(Class7Array* param1, Class8* class8Param) This should take one more parameter for 'this' > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:177 > + m_data->invokeCallback(jsclass8Param, args, &raisedException); ... and use that here. Created attachment 137503 [details] Patch Thanks arv, I have updated patch with above comments. > Also, if there are more than 2 parameters we will invoke the callback multiple times. Done. Avoiding multiple invocation of callback. > [PassThis=WebKitMutationObserver] boolean handleEvent(in sequence<MutationRecord> mutations, in WebKitMutationObserver observer); Done. Now "this" value which needs to pass depends on PassThis="*" type irrespective of the position of arguments. Added test to TestCallback.idl verifies this behaviour. Comment on attachment 137503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137503&action=review This is pretty good at the moment. My main concern is now that this patch does 2 distinct things. PassThis should work with or without sequence. I think it would be good to split this into two bugs/patches. Sorry. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2746 > + next if $codeGenerator->GetArrayType($param->type) or $param->type ne $thisType; Why can't $thisType be an array type? next if $param->type ne $thisType; > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2964 > + push(@args, GetNativeTypeForCallbacks("${arrayType}Array") . " " . $param->name); Would it be cleaner if GetNativeTypeForCallbacks took care of whether this was an array type or not? > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3073 > + my @GenerateEventListenerImpl = (); This is not used > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:158 > + ASSERT(param1); > + if (!param1) > + return true; Do we want the same for thisClassParam? My gut feeling is that it is not necessary but an ASSERT might be good in any case. (In reply to comment #44) > (From update of attachment 137503 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=137503&action=review > > This is pretty good at the moment. My main concern is now that this patch does 2 distinct things. PassThis should work with or without sequence. > > I think it would be good to split this into two bugs/patches. Sorry. Oke filed https://bugs.webkit.org/show_bug.cgi?id=84232 > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2746 > > + next if $codeGenerator->GetArrayType($param->type) or $param->type ne $thisType; > > Why can't $thisType be an array type? Right should next if $param->type ne $thisType; > > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2964 > > + push(@args, GetNativeTypeForCallbacks("${arrayType}Array") . " " . $param->name); > > Would it be cleaner if GetNativeTypeForCallbacks took care of whether this was an array type or not? Will do. > > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:158 > > + ASSERT(param1); > > + if (!param1) > > + return true; > > Do we want the same for thisClassParam? My gut feeling is that it is not necessary but an ASSERT might be good in any case. Will do. Created attachment 137690 [details]
Updated Patch
Updated patch after bug fix 84232.
I have added test to TestCallback.idl because with sequence<T> codegenerator with callback with [PassThisToCallback] behaves differently.
Please let me know your review comments on this.
Attachment 137690 [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/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5]
Total errors found: 1 in 11 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 137696 [details]
Patch
Sorry for the previous patch :(.
Comment on attachment 137696 [details] Patch Attachment 137696 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12424562 Comment on attachment 137696 [details]
Patch
Is this still desired? If so, rebase eliminating the V8 references and resubmit.
|