Bug 161030 – Reduce the amount of custom binding code for JSXPathNSResolver

• Component: Bindings
• Status: RESOLVED FIXED

[Ryosuke Niwa]

XPathNSResolver has a lot of custom binding code because of the need to support JSCustomXPathNSResolver.
Use the callback interface for the custom callback to reduce the amount of custom binding code.

[Ryosuke Niwa]

Created attachment 286535 [details]
Patch

[Darin Adler]

Comment on attachment 286535 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4241
> +            push(@implContent, "\n" . GetNativeTypeForCallbacks($interface, $function->signature->type, 1) . " ${className}::${functionName}(");

This ",1" thing is really unclear.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4569
>  sub GetNativeTypeForCallbacks
>  {
> -    my ($interface, $type) = @_;
> +    my ($interface, $type, $isReturnType) = @_;
>  
>      return "RefPtr<SerializedScriptValue>&&" if $type eq "SerializedScriptValue";
>      return "RefPtr<DOMStringList>&&" if $type eq "DOMStringList";
> -    return "const String&" if $codeGenerator->IsStringType($type);
> +    return "const String&" if !($interface->isCallback && $isReturnType) && $codeGenerator->IsStringType($type);
>      return GetNativeType($interface, $type);
>  }

How about a separate function for return type that calls GetNativeTypeForCallbacks? Then we don’t need the integer.

[Darin Adler]

Comment on attachment 286535 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4569
>>  }
> 
> How about a separate function for return type that calls GetNativeTypeForCallbacks? Then we don’t need the integer.

GetNativeReturnTypeForCallbacks and GetNativeArgumentTypeForCallbacks. One calls the other and has any special cases that need to be different.

> Source/WebCore/xml/XPathCustomNSResolver.idl:35
> +callback interface XPathCustomNSResolver {
> +
> +    [Custom] DOMString? lookupNamespaceURI(DOMString prefix);
> +
> +};

I suggest omitting those blank lines.

[Sam Weinig]

Comment on attachment 286535 [details]
Patch

Marking r- since it doesn't apply cleanly, but it generally looks good.

[Ryosuke Niwa]

Created attachment 286624 [details]
Patch

[Chris Dumez]

Comment on attachment 286624 [details]
Patch

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

> Source/WebCore/xml/CustomXPathNSResolver.idl:31
> +callback interface CustomXPathNSResolver {

It does not seem right to avoid both CustomXPathNSResolver and XPathNSResolver. Shouldn't we make XPathNSResolver a callback interface instead?

> Source/WebCore/xml/XPathEvaluator.idl:26
> +    [RaisesException] XPathExpression createExpression(DOMString expression, CustomXPathNSResolver resolver);

These overloads do not look right.

[Chris Dumez]

Comment on attachment 286624 [details]
Patch

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

>> Source/WebCore/xml/CustomXPathNSResolver.idl:31
>> +callback interface CustomXPathNSResolver {
> 
> It does not seem right to avoid both CustomXPathNSResolver and XPathNSResolver. Shouldn't we make XPathNSResolver a callback interface instead?

"to have both"

[Chris Dumez]

See for e.g. what I did for NodeFilter in:
https://bugs.webkit.org/show_bug.cgi?id=148415

[Ryosuke Niwa]

In the case of XMLNSResolver, there's a WebKit/C++ implementation of it as well, and author can explicitly create it.

So we need to support both the author created callback interface as well as a wrapper.

[Chris Dumez]

(In reply to comment #9)
> In the case of XMLNSResolver, there's a WebKit/C++ implementation of it as
> well, and author can explicitly create it.
> 
> So we need to support both the author created callback interface as well as
> a wrapper.

AFAIK, it is the same for NodeFilter?

[Chris Dumez]

(In reply to comment #10)
> (In reply to comment #9)
> > In the case of XMLNSResolver, there's a WebKit/C++ implementation of it as
> > well, and author can explicitly create it.
> > 
> > So we need to support both the author created callback interface as well as
> > a wrapper.
> 
> AFAIK, it is the same for NodeFilter?

What prevents a callback interface from having a C++ implementation?

[Chris Dumez]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > In the case of XMLNSResolver, there's a WebKit/C++ implementation of it as
> > > well, and author can explicitly create it.
> > > 
> > > So we need to support both the author created callback interface as well as
> > > a wrapper.
> > 
> > AFAIK, it is the same for NodeFilter?
> 
> What prevents a callback interface from having a C++ implementation?

Ryosuke showed me it would be a lot of work to make XMLNSResolver an actual callback interface so the split seems fine then.

[Ryosuke Niwa]

Comment on attachment 286624 [details]
Patch

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

>> Source/WebCore/xml/XPathEvaluator.idl:26
>> +    [RaisesException] XPathExpression createExpression(DOMString expression, CustomXPathNSResolver resolver);
> 
> These overloads do not look right.

They can’t be optional because then it would be ambiguous as to which C++ method needs to be called.

[Ryosuke Niwa]

Created attachment 286670 [details]
Updated for ToT

[Ryosuke Niwa]

Created attachment 286678 [details]
Fixed Windows build

[Chris Dumez]

Patch does not build on GTK and Windows.

[Ryosuke Niwa]

Created attachment 286831 [details]
Updated for ToT

[Ryosuke Niwa]

Created attachment 286832 [details]
Fixed Windows build

[Ryosuke Niwa]

Created attachment 287377 [details]
Fixed GTK+ build

[Ryosuke Niwa]

I don't think GTK+ build failure is related to my patch:
ccache: error: Failed to create temporary file for /home/tanty/.ccache/1/7/d1d31a71b272d0954b437e4fae1ff6-813.o.tmp.stderr: No space left on device

[Chris Dumez]

Comment on attachment 287377 [details]
Fixed GTK+ build

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1965
> +                if ($codeGenerator->IsCallbackInterface($type)) {

This does not look right. As per the specification, callback interfaces should be handled with dictionaries below. See isDictionaryOrObjectOrCallbackInterfaceParameter check below.

[Chris Dumez]

Comment on attachment 287377 [details]
Fixed GTK+ build

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4620
> +    return GetNativeReturnTypeForCallbacks($interface, $type);

It seems weird to call GetNativeReturnTypeForCallbacks from GetNativeArgumentTypeForCallbacks.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4627
> +    return "RefPtr<SerializedScriptValue>&&" if $type eq "SerializedScriptValue";

I don't think we would have a RefPtr<>&& as a return value, but rather a RefPtr<>

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4628
> +    return "const String&" if !$interface->isCallback && $codeGenerator->IsStringType($type);

Could you clarify why we need to distinguish callback interfaces here? Maybe it would be simpler to always return a String and only use "const String&" for arguments.

> Source/WebCore/xml/CustomXPathNSResolver.idl:33
> +callback interface CustomXPathNSResolver {

I think we should find a clearer naming for this interface. I suggest XPathNSResolverCallback.

[Chris Dumez]

Comment on attachment 287377 [details]
Fixed GTK+ build

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

> Source/WebCore/xml/CustomXPathNSResolver.h:38
> +class CustomXPathNSResolver : public XPathNSResolver {

Something is weird here. Why do we need this class? JSCustomXPathNSResolver is already a subclass of XPathNSResolver AFAIK.

[Ryosuke Niwa]

I've spent way too much time on this patch already. Not gonna work on it.

[Sam Weinig]

This should not be closed, it is still something that needs to be done.

[Alexey Shvayka]

Created attachment 406107 [details]
Patch

[Darin Adler]

Comment on attachment 406107 [details]
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.

[Alexey Shvayka]

Created attachment 406158 [details]
Patch

Fix tests and (hopefully) GTK build, use `auto`, improve note on [IsWeakCallback].

[Alexey Shvayka]

(In reply to Darin Adler from comment #27)

Thank you for review, Darin!

> I’d expect you could use *functionName.uid() rather than
> String(functionName.uid()) and it might even be slightly more efficient.

*functionName.uid() didn't compile; other occurrences of makeString() with a property key seem to also use String().

> If not, then fine to land like this, but someone should probably fix makeString
> so it can take a StringImpl* or StringImpl&.

That would be a great improvement.

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

I've checked other DOM callbacks, and it seems like a convention.

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

Unfortunately, "lookUp" doesn't match DOM casing that is pretty spread over several classes.

[EWS]

Committed r265387: <https://trac.webkit.org/changeset/265387>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406158 [details].

[Radar WebKit Bug Importer]

<rdar://problem/66694615>