Bug 8791 – XPath should support custom namespace resolvers

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Anders Carlsson]

XPathEvaluator.evaluate should be able to take a custom node resolver instead of one that's been created by using XPathEvaluator.createNSResolver. The spec says:

The parameter resolver of the method XPathEvaluator.evaluate is specified as an object that implements the XPathNSResolver interface. ECMAScript users can also pass to this method a function which returns a String and takes a String parameter instead of the resolver parameter.

[Jeremy McPeak]

Created attachment 10015 [details]
A quick HTML file showing the use of a user-defined function for the namespace resolver. Works in Gecko and Opera.

The W3C specification states that user-defined functions can be used as namespace resolvers in XPathEvaluator.evaluate() for ECMAScript. This test case shows this usage, and works in Gecko and Opera. When ran in WebKit, a NAMESPACE_ERR error is thrown.

[Alexey Proskuryakov]

*** Bug 12413 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

Created attachment 12892 [details]
proposed fix

[Darin Adler]

Comment on attachment 12892 [details]
proposed fix

Looks good.

I don't see make file changes for the platforms other than Macintosh.

+String JSCustomXPathNSResolver::lookupNamespaceURI(const String& prefix)
+{
+    String result;

Would be better to declare this closer to where it's used.

+    RefPtr<JSCustomXPathNSResolver> selfProtector(this);

Is there a way to avoid this? Can we just get the values out of member variables and then not access this after calling the function?

Is there a way to cut down on the amount of custom code that's in the CodeGeneratorJS.pm and CodeGeneratorObjC.pm scripts? Can we make helper functions for the JSCustomXPathNSResolver path so that a little more of the work is in a .cpp file rather than inside a script?

The header guards in NativeXPathNSResolver.h say JSNativeXPathNSResolver_h.

I'm going to say review- even though this is very close to a review+.

[Alexey Proskuryakov]

Created attachment 12910 [details]
proposed fix

(In reply to comment #4)

> I don't see make file changes for the platforms other than Macintosh.

  Done.

> Would be better to declare this closer to where it's used.

  Moved it a little down.

> +    RefPtr<JSCustomXPathNSResolver> selfProtector(this);
> 
> Is there a way to avoid this? Can we just get the values out of member
> variables and then not access this after calling the function?

  I don't know for sure - took it from JSAbstractEventListener::handleEvent(). I suppose it may be needed to protect the frame. Would be nice if these functions could share more code some day, but I don't feel myself familiar enough with this code to refactor it.

> Is there a way to cut down on the amount of custom code that's in the
> CodeGeneratorJS.pm and CodeGeneratorObjC.pm scripts? Can we make helper
> functions for the JSCustomXPathNSResolver path so that a little more of the
> work is in a .cpp file rather than inside a script?

  I added a create() function to JSCustomXPathNSResolver, and that saved a few lines in generated code, but not much - so maybe I don't fully understand your suggestion.

> The header guards in NativeXPathNSResolver.h say JSNativeXPathNSResolver_h.

  Fixed.

  Also added BLOCK_OBJC_EXCEPTIONS to the ObjC resolver.

[Darin Adler]

Comment on attachment 12910 [details]
proposed fix

When it comes to reducing the amount of code in CodeGeneratorJS.pm, I'm suggesting that JSCustomXPathNSResolver have a function that can do this entire job:

+                    push(@implContent, "        RefPtr<XPathNSResolver> customResolver;\n");
+                    push(@implContent, "        XPathNSResolver* resolver = toXPathNSResolver(args[$paramIndex]);\n");
+                    push(@implContent, "        if (!resolver) {\n");
+                    push(@implContent, "            customResolver = JSCustomXPathNSResolver::create(exec, args[$paramIndex]);\n");
+                    push(@implContent, "            if (exec->hadException())\n");
+                    push(@implContent, "                return jsUndefined();\n");
+                    push(@implContent, "            resolver = customResolver.get();\n");
+                    push(@implContent, "        }\n");

so that CodeGeneratorJS.pm has just a single function call, check for exception/null and return of jsUndefined(), which amounts to about half as much code as what's there now.

Seems like you could do something similar with CodeGeneratorObjC.pm, with a single function that returns a PassRefPtr<XPathNSResolver>.

+#include "XPathNSResolver.h"
+
+#include <wtf/PassRefPtr.h>
+#include <wtf/RefPtr.h>

This requires only Forward.h, not PassRefPtr.h (occurs twice).

r=me

[Alexey Proskuryakov]

Committed revision 19398.

(In reply to comment #6)
> so that CodeGeneratorJS.pm has just a single function call, check for
> exception/null and return of jsUndefined(), which amounts to about half as much
> code as what's there now.

  I see. I didn't want to make JSCustomXPathNSResolver know about wrapped native resolvers - seems outside its area of responsibility.

> This requires only Forward.h, not PassRefPtr.h (occurs twice).

  Fixed.

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.