Bug 161030

Summary: Reduce the amount of custom binding code for JSXPathNSResolver
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: BindingsAssignee: Alexey Shvayka <ashvayka>
Status: RESOLVED FIXED    
Severity: Trivial CC: annulen, ashvayka, cdumez, darin, esprehn+autocc, ews-watchlist, gyuyoung.kim, kondapallykalyan, ryuan.choi, sam, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar, WebExposed
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=148415
Bug Depends on:    
Bug Blocks: 160926    
Attachments:
Description Flags
Patch
none
Patch
none
Updated for ToT
none
Fixed Windows build
none
Updated for ToT
none
Fixed Windows build
none
Fixed GTK+ build
cdumez: review-
Patch
darin: review+
Patch none

Description Ryosuke Niwa 2016-08-20 00:27:49 PDT
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.
Comment 1 Ryosuke Niwa 2016-08-20 00:38:55 PDT
Created attachment 286535 [details]
Patch
Comment 2 Darin Adler 2016-08-20 18:26:21 PDT
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.
Comment 3 Darin Adler 2016-08-20 18:29:17 PDT
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.
Comment 4 Sam Weinig 2016-08-21 12:39:20 PDT
Comment on attachment 286535 [details]
Patch

Marking r- since it doesn't apply cleanly, but it generally looks good.
Comment 5 Ryosuke Niwa 2016-08-22 14:58:15 PDT
Created attachment 286624 [details]
Patch
Comment 6 Chris Dumez 2016-08-22 15:13:34 PDT
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.
Comment 7 Chris Dumez 2016-08-22 15:16:00 PDT
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"
Comment 8 Chris Dumez 2016-08-22 15:19:22 PDT
See for e.g. what I did for NodeFilter in:
https://bugs.webkit.org/show_bug.cgi?id=148415
Comment 9 Ryosuke Niwa 2016-08-22 16:01:57 PDT
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.
Comment 10 Chris Dumez 2016-08-22 16:03:28 PDT
(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?
Comment 11 Chris Dumez 2016-08-22 16:03:57 PDT
(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?
Comment 12 Chris Dumez 2016-08-22 16:53:52 PDT
(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.
Comment 13 Ryosuke Niwa 2016-08-22 16:56:59 PDT
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.
Comment 14 Ryosuke Niwa 2016-08-22 22:01:30 PDT
Created attachment 286670 [details]
Updated for ToT
Comment 15 Ryosuke Niwa 2016-08-22 22:49:03 PDT
Created attachment 286678 [details]
Fixed Windows build
Comment 16 Chris Dumez 2016-08-23 08:36:37 PDT
Patch does not build on GTK and Windows.
Comment 17 Ryosuke Niwa 2016-08-23 21:30:30 PDT
Created attachment 286831 [details]
Updated for ToT
Comment 18 Ryosuke Niwa 2016-08-23 21:32:41 PDT
Created attachment 286832 [details]
Fixed Windows build
Comment 19 Ryosuke Niwa 2016-08-30 00:25:48 PDT
Created attachment 287377 [details]
Fixed GTK+ build
Comment 20 Ryosuke Niwa 2016-08-30 18:43:05 PDT
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
Comment 21 Chris Dumez 2016-08-31 09:00:45 PDT
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.
Comment 22 Chris Dumez 2016-08-31 09:13:50 PDT
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.
Comment 23 Chris Dumez 2016-08-31 09:19:06 PDT
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.
Comment 24 Ryosuke Niwa 2016-08-31 10:34:31 PDT
I've spent way too much time on this patch already. Not gonna work on it.
Comment 25 Sam Weinig 2016-09-02 13:49:00 PDT
This should not be closed, it is still something that needs to be done.
Comment 26 Alexey Shvayka 2020-08-06 13:16:04 PDT
Created attachment 406107 [details]
Patch
Comment 27 Darin Adler 2020-08-06 14:02:16 PDT
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.
Comment 28 Alexey Shvayka 2020-08-07 00:28:01 PDT
Created attachment 406158 [details]
Patch

Fix tests and (hopefully) GTK build, use `auto`, improve note on [IsWeakCallback].
Comment 29 Alexey Shvayka 2020-08-07 12:13:04 PDT
(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.
Comment 30 EWS 2020-08-07 12:28:01 PDT
Committed r265387: <https://trac.webkit.org/changeset/265387>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406158 [details].
Comment 31 Radar WebKit Bug Importer 2020-08-07 12:28:23 PDT
<rdar://problem/66694615>