Bug 12733 - XPath namespace is attached to a wrong step
Summary: XPath namespace is attached to a wrong step
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Alexey Proskuryakov
Depends on:
Blocks: 12584
  Show dependency treegraph
Reported: 2007-02-11 01:47 PST by Alexey Proskuryakov
Modified: 2007-02-17 14:21 PST (History)
0 users

See Also:

proposed fix (11.20 KB, patch)
2007-02-11 03:06 PST, Alexey Proskuryakov
mjs: review-
Details | Formatted Diff | Diff
proposed fix (11.58 KB, patch)
2007-02-13 12:45 PST, Alexey Proskuryakov
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2007-02-11 01:47:18 PST
When parsing paths such as "//svg:g[@id = 'r_00']", the namespace gets attached to the predicate, incorrectly resulting in something like "//g[@svg:id = 'r_00']".
Comment 1 Alexey Proskuryakov 2007-02-11 03:06:10 PST
Created attachment 13116 [details]
proposed fix

Suggestions are welcome on how to reduce code duplication in XPathGrammar.y. I thought about changing the type of NodeTest to a (string, namespace) pair, but this seemed unfair to other tests that do not use a namespace.
Comment 2 Maciej Stachowiak 2007-02-11 19:18:54 PST
Comment on attachment 13116 [details]
proposed fix

You could eliminate the duplication by adding an OptionalPredicateList production. r- to consider this, although your fix seems fine as-is.
Comment 3 Alexey Proskuryakov 2007-02-13 12:45:08 PST
Created attachment 13152 [details]
proposed fix

Thank you - did that!
Comment 4 Darin Adler 2007-02-15 10:03:00 PST
Comment on attachment 13152 [details]
proposed fix

The only reason I haven't marked this patch reviewed yet is that I'm not yet sure about the storage management in XPathGrammar.y -- I don't understand the rules about when a call to deleteString is needed and when it's not and I'd need to before I could mark this reviewed.
Comment 5 Maciej Stachowiak 2007-02-17 08:20:22 PST
Comment on attachment 13152 [details]
proposed fix

Darin has a good point. As it is, it looks like the YYABORT calls may leak. On the other hand, it looks to me like none of this is new with Alexey's patch. It might be worth investigating separately.

So r=me
Comment 6 Sam Weinig 2007-02-17 14:21:54 PST
Landed in r19684.