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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Apr 5 09:54:36 PDT 2011


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





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

>> Source/WebCore/bindings/js/CallbackUtilities.h:25
>> +#ifndef CallbackUtilities_h
> 
> We frown on terms like “utilities” and “manager” in our filenames and class names. These  excess words don’t add much clarity and in the case of classes, are a bit of an object oriented programming anti-pattern.
> 
> It looks like some have crept over time, which is unfortunate: IDBBindingUtilities, DOMUtility, V8Utilities, c_utility.cpp, JNIUtility, objc_utility, AudioUtilities, ClipboardUtilitiesChromium, ClipboardUtilitiesWin, SVGParserUtilities, InjectedScriptManager, BreakpointManager, TextureManager, ResourceHandleManager. You’ll notice that most of these are on recent additions to WebKit, so probably just folks who didn’t participate in the consensus view that we want to avoid those vague words.
> 
> For this header file, I would suggest the name Callback.h or CallbackFunction.h or FunctionOnlyCallback.h.

Changed to CallbackFunction.h as this option should keep the previous ordering in the project files.

>>> Source/WebCore/bindings/js/CallbackUtilities.h:42
>>> +// 'FunctionOnly' is assumed for the created callback. Callable objects created via JSC API are disallowed.
>> 
>> Comments need to answer the question “why?” and this no longer makes it clear that the “callable objects are disallowed” behavior is possibly a bug. I know we still have FIXMEs at the call site, but it seems wrong to make a function that has behavior that’s probably not helpful anywhere, but comment on it in a way that makes it seem deliberate.
>> 
>> So I’d like to see a comment that’s more clear on this point. And I think that having a FIXME at each call site is not really all that helpful. There’s some slight chance that some day we might change one call site and not others, but it’s more likely we’ll fix things in this central location.
>> 
>> Also, I’m something of an expert on this, but I don’t understand what FunctionOnly means. Is this a reference to something in a specification somewhere?
> 
> 'FunctionOnly' is a WebIDL tag used by Geolocation and MediaStream. There's ongoing discussion as to how we should handle this in https://bugs.webkit.org/show_bug.cgi?id=40012.
> 
> I agree that the FIXME should be in this new helper, not at each call site. The helper creates a FunctionOnly callback, and that won't change, the question is how to do this.

This 'callable objects are disallowed' comes from the spec and has been discussed and solved here: https://bugs.webkit.org/show_bug.cgi?id=40012.
I'm applying the suggested solution and removing that part from the comment.

FunctionOnly is an extended attributed of [Callback] according to the WebIDL specification: http://dev.w3.org/2006/webapi/WebIDL/#Callback

>> Source/WebCore/bindings/js/CallbackUtilities.h:53
>> +    }
> 
> If performance is an issue, then the checks for undefined and null should go inside the !inherits block. Otherwise we’ll be doing extra checks for them.
> 
> The undefined and null checks would be easier to read in separate if statements, with more straightforward formatting without that line break.

Fixed. Undefined and null checks separated. Not moved into the later check since they are not expected to set an exception, but to allow uses like creating callbacks from optional (undefined) arguments.

>> Source/WebCore/bindings/js/CallbackUtilities.h:56
>> +    return JSCallbackType::create(object, globalObject);
> 
> The local variable here doesn’t add any clarity; I suggest using asObject directly inside the function call.

Fixed.

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


More information about the webkit-unassigned mailing list