Bug 8791 - XPath should support custom namespace resolvers
Summary: XPath should support custom namespace resolvers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
: 12413 (view as bug list)
Depends on:
Blocks: 10489 12584
  Show dependency treegraph
 
Reported: 2006-05-08 14:27 PDT by Anders Carlsson
Modified: 2019-02-06 09:02 PST (History)
4 users (show)

See Also:


Attachments
A quick HTML file showing the use of a user-defined function for the namespace resolver. Works in Gecko and Opera. (1.11 KB, text/html)
2006-08-13 11:14 PDT, Jeremy McPeak
no flags Details
proposed fix (44.22 KB, patch)
2007-02-03 01:21 PST, Alexey Proskuryakov
darin: review-
Details | Formatted Diff | Diff
proposed fix (46.72 KB, patch)
2007-02-04 02:13 PST, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anders Carlsson 2006-05-08 14:27:03 PDT
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.
Comment 1 Jeremy McPeak 2006-08-13 11:14:40 PDT
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.
Comment 2 Alexey Proskuryakov 2007-01-28 11:25:17 PST
*** Bug 12413 has been marked as a duplicate of this bug. ***
Comment 3 Alexey Proskuryakov 2007-02-03 01:21:24 PST
Created attachment 12892 [details]
proposed fix
Comment 4 Darin Adler 2007-02-03 12:22:13 PST
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+.
Comment 5 Alexey Proskuryakov 2007-02-04 02:13:55 PST
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.
Comment 6 Darin Adler 2007-02-04 18:26:47 PST
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
Comment 7 Alexey Proskuryakov 2007-02-04 22:07:13 PST
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.
Comment 8 Lucas Forschler 2019-02-06 09:02:49 PST
Mass moving XML DOM bugs to the "DOM" Component.