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.
Created attachment 286535 [details] Patch
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 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 on attachment 286535 [details] Patch Marking r- since it doesn't apply cleanly, but it generally looks good.
Created attachment 286624 [details] Patch
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 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"
See for e.g. what I did for NodeFilter in: https://bugs.webkit.org/show_bug.cgi?id=148415
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.
(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?
(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?
(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 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.
Created attachment 286670 [details] Updated for ToT
Created attachment 286678 [details] Fixed Windows build
Patch does not build on GTK and Windows.
Created attachment 286831 [details] Updated for ToT
Created attachment 286832 [details] Fixed Windows build
Created attachment 287377 [details] Fixed GTK+ build
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 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 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 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.
I've spent way too much time on this patch already. Not gonna work on it.
This should not be closed, it is still something that needs to be done.
Created attachment 406107 [details] Patch
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.
Created attachment 406158 [details] Patch Fix tests and (hopefully) GTK build, use `auto`, improve note on [IsWeakCallback].
(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.
Committed r265387: <https://trac.webkit.org/changeset/265387> All reviewed patches have been landed. Closing bug and clearing flags on attachment 406158 [details].
<rdar://problem/66694615>