[Webkit-unassigned] [Bug 57770] Factoring the creation of 'FunctionOnly' callbacks in JavaScriptCore

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=57770





--- Comment #20 from Leandro Graciá Gil <leandrogracia at chromium.org>  2011-04-06 07:35:58 PST ---
(From update of attachment 88272)
View in context: https://bugs.webkit.org/attachment.cgi?id=88272&action=review

>>>> Source/WebCore/bindings/js/CallbackFunction.h:30
>>>> +#include <wtf/Forward.h>
>>> 
>>> What is wtf/Forward.h needed for? I don’t see anything here that would benefit from including that.
>> 
>> The return value of the template is a PassRefPtr object.
> 
> Yes, but including Forward.h is insufficient to compile the definition of a function that returns a PassRefPtr. It’s enough to compile a declaration, but not a definition.
> 
> The code compiles because JSFunction.h ends up including PassRefPtr.h and your include of Forward.h has no effect.

Fixed.

>>>>> Source/WebCore/bindings/js/CallbackFunction.h:35
>>>>> +    CallbackAllowFunction = 0,
>>>> 
>>>> Should probably be named CallbackAllowFunctionOnly. It’s misleading to have it be named AllowFunction when a function will be allowed even if you don’t pass this value!
>>> 
>>> I already thought that, but then it makes no sense to write something valid like CallbackAllowFunctionOnly | CallbackAllowNull. Do you think it should be better to have CallbackAllowFunctionOnly since that value is only used in the exclusive cases and ignore it in other situations?
>> 
>> Yes, I'd thought about this too. I see problems with both names, so don't feel strongly either way.
> 
> Yes, I think the real problem is more fundamental. Since these constants are designed to or together it makes no sense to have one with the value 0; you can’t or that with something else in a meaningful way. We’d be better off with no name for the value 0, and with the flags defaulting to 0. Then no caller has to pass 0 explicitly.

Fixed. Will prepare a bug to make the V8 version consistent to this.

>> Source/WebCore/bindings/js/CallbackFunction.h:57
>> +    }
> 
> There’s no real reason this has to be in the header. If this logic was in a non-template function called by the template function we could make this header include fewer header files.

Fixed.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.