Bug 30128 – [v8] fix crbug/671: XPathEvaluator.evalute() will throw "NAMESPACE_ERR: DOM Exception 14" when applied to XML with namespaces

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[anton muhin]

This patch allows automatic conversion of functions to namespace resolvers.

[anton muhin]

Created attachment 40718 [details]
First pass

I am somewhat concerned how that complicates the logic of CodeGeneratorV8.pm.  Another option I considered was to make necessary methods custom and provide hand-written implementation.  However, that won't fix other methods that might have the same problem, e.g., originally I only fixed Document.createExpression, but fix propagated to XPathEvaluator.evaluate automatically.  On the other hand, maybe we should decide for exceptions on case by case basis.  The last thing that convinced me not to go with custom methods was Safari---they have this special logic for all XPathEvaluators.

And another note.  I added new layout tests into existing .html, but if it's against the rules (e.g. we'd like to emphasize that those tests verify contra standard behaviour), I'd be glad to lift them into a separate file.

[Dimitri Glazkov (Google)]

Comment on attachment 40718 [details]
First pass

r=me.

[Eric Seidel (no email)]

Does this already work in JSC?

[Dimitri Glazkov (Google)]

Yep.

[WebKit Commit Bot]

Comment on attachment 40718 [details]
First pass

Clearing flags on attachment: 40718

Committed r49191: <http://trac.webkit.org/changeset/49191>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This was already covered by fast/xpath/nsresolver-function.xhtml and many other tests. Did you see their results change when running the tests?

Also, this is not a non-standard extension, see a note at the very bottom of <http://www.w3.org/TR/DOM-Level-3-XPath/ecma-script-binding.html>:

"Note: 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."

[anton muhin]

(In reply to comment #7)
> This was already covered by fast/xpath/nsresolver-function.xhtml and many other
> tests. Did you see their results change when running the tests?

I didn't see any changes in LayoutTest results of both WebKit and Chromium.  This nsresolver-function.xhtml apparently only tests Document.evaluate which already had necessary logic.  I am aware of two cases this patch'd have fixed: Document.createExpression and XPathEvaluator.evaluate.  Grepping through fast/xpath doesn't show tests for createExpression.

> 
> Also, this is not a non-standard extension, see a note at the very bottom of
> <http://www.w3.org/TR/DOM-Level-3-XPath/ecma-script-binding.html>:
> 
> "Note: 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."

Thank you very much for the pointer.  I wonder maybe we should alter Document.idl then?  That would make logic in codegen somewhat less involved (apparently)

[Alexey Proskuryakov]

(In reply to comment #8)
> This nsresolver-function.xhtml apparently only tests Document.evaluate which
> already had necessary logic.

I see, it's my fault then that I didn't add tests for all code paths when fixing this for JavaScriptCore. Sorry about that!

> I wonder maybe we should alter
> Document.idl then?  That would make logic in codegen somewhat less involved
> (apparently)

That may have a negative effect on non-JavaScript bindings though (if only an ifdef in IDL). Hard to tell without seeing the code changes, but maybe not something worth doing at this point, when we have this implemented already.