Bug 30128 - [v8] fix crbug/671: XPathEvaluator.evalute() will throw "NAMESPACE_ERR: DOM Exception 14" when applied to XML with namespaces
Summary: [v8] fix crbug/671: XPathEvaluator.evalute() will throw "NAMESPACE_ERR: DOM E...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-06 06:55 PDT by anton muhin
Modified: 2009-10-06 15:57 PDT (History)
5 users (show)

See Also:


Attachments
First pass (8.00 KB, patch)
2009-10-06 07:03 PDT, anton muhin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description anton muhin 2009-10-06 06:55:17 PDT
This patch allows automatic conversion of functions to namespace resolvers.
Comment 1 anton muhin 2009-10-06 07:03:43 PDT
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.
Comment 2 Dimitri Glazkov (Google) 2009-10-06 09:50:21 PDT
Comment on attachment 40718 [details]
First pass

r=me.
Comment 3 Eric Seidel (no email) 2009-10-06 09:57:28 PDT
Does this already work in JSC?
Comment 4 Dimitri Glazkov (Google) 2009-10-06 09:58:09 PDT
Yep.
Comment 5 WebKit Commit Bot 2009-10-06 10:15:30 PDT
Comment on attachment 40718 [details]
First pass

Clearing flags on attachment: 40718

Committed r49191: <http://trac.webkit.org/changeset/49191>
Comment 6 WebKit Commit Bot 2009-10-06 10:15:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Alexey Proskuryakov 2009-10-06 13:43:21 PDT
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."
Comment 8 anton muhin 2009-10-06 14:10:50 PDT
(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)
Comment 9 Alexey Proskuryakov 2009-10-06 15:57:59 PDT
(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.