[webkit-reviews] review granted: [Bug 80573] Support [Custom] for static functions : [Attachment 130781] Patch made with -w

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Mar 8 16:43:04 PST 2012


Kentaro Hara <haraken at chromium.org> has granted  review:
Bug 80573: Support [Custom] for static functions
https://bugs.webkit.org/show_bug.cgi?id=80573

Attachment 130781: Patch made with -w
https://bugs.webkit.org/attachment.cgi?id=130781&action=review

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
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.


More information about the webkit-reviews mailing list