[webkit-reviews] review granted: [Bug 57770] Factoring the creation of 'FunctionOnly' callbacks in JavaScriptCore : [Attachment 88244] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Leandro Graciá Gil
<leandrogracia at chromium.org>'s request for review:
Bug 57770: Factoring the creation of 'FunctionOnly' callbacks in JavaScriptCore
https://bugs.webkit.org/show_bug.cgi?id=57770

Attachment 88244: Patch
https://bugs.webkit.org/attachment.cgi?id=88244&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=88244&action=review

Since this patch is entirely about refactoring and not a behavior change,
stylistic and clarity issues are paramount, more important even than with a bug
fix. So please read my comments with that in mind.

I’m going to say review+ but I strongly suggest making at least some changes in
response to my comments even though with my review+ that is not formally
required.

> 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.

> 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?

> Source/WebCore/bindings/js/CallbackUtilities.h:53
> +    if ((value.isUndefined() && (acceptedValues & CallbackAllowUndefined))
> +	   || (value.isNull() && (acceptedValues & CallbackAllowNull)))
> +	   return 0;
> +
> +    if (!value.inherits(&JSC::JSFunction::s_info)) {
> +	   setDOMException(exec, TYPE_MISMATCH_ERR);
> +	   return 0;
> +    }

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.

> Source/WebCore/bindings/js/CallbackUtilities.h:56
> +    JSC::JSObject* object = asObject(value);
> +    return JSCallbackType::create(object, globalObject);

The local variable here doesn’t add any clarity; I suggest using asObject
directly inside the function call.