| Summary: | Support [Custom] for static functions | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jon Lee <jonlee> | ||||
| Component: | WebCore JavaScript | Assignee: | Jon Lee <jonlee> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | abarth, haraken, japhet, ojan, webkit.review.bot, yaar | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Mac | ||||||
| OS: | OS X 10.7 | ||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 80472, 80485 | ||||||
| Attachments: |
|
||||||
|
Description
Jon Lee
2012-03-07 23:22:39 PST
Created attachment 130781 [details]
Patch made with -w
To make it easier to evaluate the patch, I ran the diff with the -w flag. This should fail the style bot, but the actual diff is stylistically correct.
Comment on attachment 130781 [details] Patch made with -w View in context: https://bugs.webkit.org/attachment.cgi?id=130781&action=review Just an out-of-curious question: What method wants to use [Custom] static? > Source/WebCore/ChangeLog:32 > + * bindings/scripts/test/JS/JSTestObj.cpp: > + (WebCore): > + (WebCore::jsTestObjConstructorFunctionClassMethod2): > + * bindings/scripts/test/JS/JSTestObj.h: > + (JSTestObj): > + (WebCore): > + * bindings/scripts/test/ObjC/DOMTestObj.h: > + * bindings/scripts/test/ObjC/DOMTestObj.mm: > + (-[DOMTestObj classMethod2]): > + * bindings/scripts/test/V8/V8TestObj.cpp: > + (WebCore::ConfigureV8TestObjTemplate): > + * bindings/scripts/test/V8/V8TestObj.h: > + (V8TestObj): It seems you also touched CodeGenerator{V8,ObjC}.pm, but they are not included in this patch. Would you please update the patch correctly? BTW, don't we need to modify CodeGenerator{GObject,CPP}.pm? (In reply to comment #2) > (From update of attachment 130781 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=130781&action=review > > Just an out-of-curious question: What method wants to use [Custom] static? This would be for bug 80485, where the permission functions are attached to the Notification object itself. > > Source/WebCore/ChangeLog:32 > > + * bindings/scripts/test/JS/JSTestObj.cpp: > > + (WebCore): > > + (WebCore::jsTestObjConstructorFunctionClassMethod2): > > + * bindings/scripts/test/JS/JSTestObj.h: > > + (JSTestObj): > > + (WebCore): > > + * bindings/scripts/test/ObjC/DOMTestObj.h: > > + * bindings/scripts/test/ObjC/DOMTestObj.mm: > > + (-[DOMTestObj classMethod2]): > > + * bindings/scripts/test/V8/V8TestObj.cpp: > > + (WebCore::ConfigureV8TestObjTemplate): > > + * bindings/scripts/test/V8/V8TestObj.h: > > + (V8TestObj): > > It seems you also touched CodeGenerator{V8,ObjC}.pm, but they are not included in this patch. Would you please update the patch correctly? BTW, don't we need to modify CodeGenerator{GObject,CPP}.pm? Those are just what the scripts produced as a result of the new test case. I did not change the generation scripts. But you're right for V8; I see some missing generated code, so I'll work on that. I might need some assistance because I'm unfamiliar with that area. Looking at the existing test output code for ObjC, GObject, and CPP, it looks like static isn't used for those languages, because the generated code is already incorrect. I will file bugs to track that work, but don't think it belongs in this bug since it just extends the existing static generation code. > >
> > It seems you also touched CodeGenerator{V8,ObjC}.pm, but they are not included in this patch. Would you please update the patch correctly? BTW, don't we need to modify CodeGenerator{GObject,CPP}.pm?
>
> Those are just what the scripts produced as a result of the new test case. I did not change the generation scripts. But you're right for V8; I see some missing generated code, so I'll work on that. I might need some assistance because I'm unfamiliar with that area.
Actually, it seems that since this is a custom static method, that the output is correct. Someone else would be responsible for implementing that callback, right? Or am I missing something here?
Comment on attachment 130781 [details]
Patch made with -w
Resetting to r? based on my comments and questions.
Comment on attachment 130781 [details] Patch made with -w View in context: https://bugs.webkit.org/attachment.cgi?id=130781&action=review >>> Source/WebCore/ChangeLog:32 >>> + (V8TestObj): >> >> It seems you also touched CodeGenerator{V8,ObjC}.pm, but they are not included in this patch. Would you please update the patch correctly? BTW, don't we need to modify CodeGenerator{GObject,CPP}.pm? > > Those are just what the scripts produced as a result of the new test case. I did not change the generation scripts. But you're right for V8; I see some missing generated code, so I'll work on that. I might need some assistance because I'm unfamiliar with that area. > > Looking at the existing test output code for ObjC, GObject, and CPP, it looks like static isn't used for those languages, because the generated code is already incorrect. I will file bugs to track that work, but don't think it belongs in this bug since it just extends the existing static generation code. Sorry for the confusion. You are right. In my understanding, - We need to modify CodeGeneratorJS.pm. - We do not need to modify CodeGeneratorV8.pm, since it has already supported [Custom] static, fortunately. - We do not need to modify CodeGenerator{ObjC, GObject, CPP}.pm, since they have not yet supported custom bindings. Is my understanding correct? > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2094 > + if ($function->isStatic) { > + if ($isCustom) { > + GenerateArgumentsCountCheck(\@implContent, $function, $dataNode); > + push(@implContent, " return JSValue::encode(${className}::" . $functionImplementationName . "(exec));\n"); > + } else { > + GenerateArgumentsCountCheck(\@implContent, $function, $dataNode); > + > + if (@{$function->raisesExceptions}) { > + push(@implContent, " ExceptionCode ec = 0;\n"); > + } > + > + my $numParameters = @{$function->parameters}; > + my ($functionString, $dummy) = GenerateParametersCheck(\@implContent, $function, $dataNode, $numParameters, $implClassName, $functionImplementationName, $svgPropertyType, $svgPropertyOrListPropertyType, $svgListPropertyType); > + GenerateImplementationFunctionCall($function, $functionString, " ", $svgPropertyType, $implClassName); > + } Can't we remove this block? It seems this block is just a duplication of the line 2119 -- 2153. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2098 > if ($interfaceName eq "DOMWindow") { > push(@implContent, " $className* castedThis = toJSDOMWindow(exec->hostThisValue().toThisObject(exec));\n"); > push(@implContent, " if (!castedThis)\n"); Nit: Indentation seems incorrect around here. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2154 > + } Nit: Indentation seems incorrect around here. Comment on attachment 130781 [details] Patch made with -w View in context: https://bugs.webkit.org/attachment.cgi?id=130781&action=review >>>> Source/WebCore/ChangeLog:32 >>>> + (V8TestObj): >>> >>> It seems you also touched CodeGenerator{V8,ObjC}.pm, but they are not included in this patch. Would you please update the patch correctly? BTW, don't we need to modify CodeGenerator{GObject,CPP}.pm? >> >> Those are just what the scripts produced as a result of the new test case. I did not change the generation scripts. But you're right for V8; I see some missing generated code, so I'll work on that. I might need some assistance because I'm unfamiliar with that area. >> >> Looking at the existing test output code for ObjC, GObject, and CPP, it looks like static isn't used for those languages, because the generated code is already incorrect. I will file bugs to track that work, but don't think it belongs in this bug since it just extends the existing static generation code. > > Sorry for the confusion. You are right. In my understanding, > > - We need to modify CodeGeneratorJS.pm. > - We do not need to modify CodeGeneratorV8.pm, since it has already supported [Custom] static, fortunately. > - We do not need to modify CodeGenerator{ObjC, GObject, CPP}.pm, since they have not yet supported custom bindings. > > Is my understanding correct? Yes this is correct. >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2094 >> + } > > Can't we remove this block? It seems this block is just a duplication of the line 2119 -- 2153. Yes, it is a simplification of 2119-2153. The code as it is right now is really hard to read because it is peppered with $function->isStatic checks. Because of the relative simplicity of static implementations, I think it's much more readable to first separate on whether the function is static. >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2098 >> push(@implContent, " if (!castedThis)\n"); > > Nit: Indentation seems incorrect around here. The wonky indentation is because I generated this diff with the -w flag, which means to ignore whitespace. Without it, the diff looks really confusing and it's hard to tell what is going on. The actual code that I will check in has correct indentation. >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2154 >> + } > > Nit: Indentation seems incorrect around here. See above comment. Comment on attachment 130781 [details] Patch made with -w View in context: https://bugs.webkit.org/attachment.cgi?id=130781&action=review >>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2094 >>> + } >> >> Can't we remove this block? It seems this block is just a duplication of the line 2119 -- 2153. > > Yes, it is a simplification of 2119-2153. The code as it is right now is really hard to read because it is peppered with $function->isStatic checks. Because of the relative simplicity of static implementations, I think it's much more readable to first separate on whether the function is static. I am not super excited at the simplification. This code is just a subset of the line 2119 -- 2153. I would guess that you just need to insert/remove "if($function->isStatic)" to/from the line 2119 -- 2153, right? >>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2098 >>> push(@implContent, " if (!castedThis)\n"); >> >> Nit: Indentation seems incorrect around here. > > The wonky indentation is because I generated this diff with the -w flag, which means to ignore whitespace. Without it, the diff looks really confusing and it's hard to tell what is going on. The actual code that I will check in has correct indentation. OK Comment on attachment 130781 [details] Patch made with -w View in context: https://bugs.webkit.org/attachment.cgi?id=130781&action=review >>>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2094 >>>> + } >>> >>> Can't we remove this block? It seems this block is just a duplication of the line 2119 -- 2153. >> >> Yes, it is a simplification of 2119-2153. The code as it is right now is really hard to read because it is peppered with $function->isStatic checks. Because of the relative simplicity of static implementations, I think it's much more readable to first separate on whether the function is static. > > I am not super excited at the simplification. This code is just a subset of the line 2119 -- 2153. I would guess that you just need to insert/remove "if($function->isStatic)" to/from the line 2119 -- 2153, right? I am not exactly sure what you're suggesting. If you expand out the entire section, you need to have that "if" clause around (using the line #s on the right hand side) 2120, then 2122-2130, then 2138-2143. And keep the existing unless and if clauses beforehand, and add additional checks on 2096 and 2100. In fact, if the function is static, we're ignoring way more lines than we are including. All of that code concerns instances and instance functions. A static function, aside from checking required parameters, should just call the static function on the WebCore class. It's really hard to debug with the code as it is because you have to keep track of whether it's static, which I think is a more important aspect to segregate on. And I think because of its simpler functionality, we should just pull it out. I am not really concerned at all about the small amount of duplicate code. Comment on attachment 130781 [details] Patch made with -w View in context: https://bugs.webkit.org/attachment.cgi?id=130781&action=review >>>>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2094 >>>>> + } >>>> >>>> Can't we remove this block? It seems this block is just a duplication of the line 2119 -- 2153. >>> >>> Yes, it is a simplification of 2119-2153. The code as it is right now is really hard to read because it is peppered with $function->isStatic checks. Because of the relative simplicity of static implementations, I think it's much more readable to first separate on whether the function is static. >> >> I am not super excited at the simplification. This code is just a subset of the line 2119 -- 2153. I would guess that you just need to insert/remove "if($function->isStatic)" to/from the line 2119 -- 2153, right? > > I am not exactly sure what you're suggesting. If you expand out the entire section, you need to have that "if" clause around (using the line #s on the right hand side) 2120, then 2122-2130, then 2138-2143. And keep the existing unless and if clauses beforehand, and add additional checks on 2096 and 2100. In fact, if the function is static, we're ignoring way more lines than we are including. All of that code concerns instances and instance functions. > > A static function, aside from checking required parameters, should just call the static function on the WebCore class. It's really hard to debug with the code as it is because you have to keep track of whether it's static, which I think is a more important aspect to segregate on. And I think because of its simpler functionality, we should just pull it out. I am not really concerned at all about the small amount of duplicate code. Makes sense. Thank you for the clarification. Committed r110232: <http://trac.webkit.org/changeset/110232> |