[webkit-reviews] review granted: [Bug 161030] Reduce the amount of custom binding code for JSXPathNSResolver : [Attachment 406107] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Shvayka
<shvaikalesh at gmail.com>'s request for review:
Bug 161030: Reduce the amount of custom binding code for JSXPathNSResolver
https://bugs.webkit.org/show_bug.cgi?id=161030

Attachment 406107: Patch

https://bugs.webkit.org/attachment.cgi?id=406107&action=review




--- Comment #27 from Darin Adler <darin at apple.com> ---
Comment on attachment 406107
  --> https://bugs.webkit.org/attachment.cgi?id=406107
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406107&action=review

> Source/WebCore/bindings/js/JSCallbackData.cpp:73
> +	       returnedException = JSC::Exception::create(vm,
createTypeError(lexicalGlobalObject, makeString("'",
String(functionName.uid()), "' property of callback interface should be
callable")));

I’d expect you could use *functionName.uid() rather than
String(functionName.uid()) and it might even be slightly more efficient. If
not, then fine to land like this, but someone should probably fix makeString so
it can take a StringImpl* or StringImpl&.

> Source/WebCore/bindings/js/JSDOMConvertXPathNSResolver.h:46
> +	   JSC::JSObject* object = asObject(value);

auto?

> Source/WebCore/xml/CustomXPathNSResolver.h:37
> +    using ActiveDOMCallback::ActiveDOMCallback;

I’m not entirely sure what this does.

Since CustomXPathNSResolver is an abstract base class, the constructor doesn’t
need to be public, so if this is a constructor it can be protected instead.

> Source/WebCore/xml/CustomXPathNSResolver.h:41
> +    virtual CallbackResult<String> lookupNamespaceURIForBindings(const
String& prefix) = 0;
> +
> +    String lookupNamespaceURI(const String& prefix);

Since lookup is a noun, I suggest considering naming these lookUp, although
that might be unpleasantly confusing if it doesn’t match the casing of the
thing in the DOM.

> Source/WebCore/xml/CustomXPathNSResolver.idl:26
> +// JSCustomXPathNSResolvers are always temporary so using a Strong reference
is safe here.

This comment would be better if it said something like "so IsWeakCallback
property is not needed here". I was looking at the file to see where it said
"Strong" and had to read the change log to figure it out.