Closed Bug 343307 Opened 18 years ago Closed 18 years ago

[FIX]getElementsByTagNameNS("*", "foo") misses nodes

Categories

(Core :: DOM: Core & HTML, defect, P1)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

In particular, with the patch for bug 206053, getElementsByTagNameNS("*", "foo") does the same thing as getElementsByTagName("foo"), which is wrong.  The problem is that we're using kNameSpaceID_Unknown to mean both cases here...

The two options I see are either to introduce another boolean constructor arg (and member) to indicate whether kNameSpaceID_Unknown means "wildcard namespace" or "non-namespace-aware match" or to have a kNameSpaceID_Wildcard constant (equal to -2 or something?).

Preferences?

I should note that I found this while trying to write some content list unit tests... so we could really use this while regression test thing.  ;)
I'll fix this when I get back, in any case.  But I'd still love opinions on the two solutions.
Flags: blocking1.9a1+
Priority: -- → P1
When this is fixed, need to update http://lxr.mozilla.org/seamonkey/source/content/test/unit/test_nodelist.js accordingly.
I sort of like the "wildcard namespace" idea, however don't make it an "official" namespace registered in nsNameSpaceManager.h, but rather just a local enum or such in nsContentList.h
Attached patch Fix (obsolete) — Splinter Review
Attachment #229404 - Flags: superreview?(bugmail)
Attachment #229404 - Flags: review?(bugmail)
Summary: getElementsByTagNameNS("*", "foo") misses nodes → [FIX]getElementsByTagNameNS("*", "foo") misses nodes
Comment on attachment 229404 [details] [diff] [review]
Fix

>Index: content/base/public/nsINameSpaceManager.h

>+// through to any of their callees who might not expect it.  Note that we don't
>+// want PR_INT32_MIN here because we want this to stay negative when 1 is
>+// subtracted.
>+#define kNameSpaceID_Reserved (PR_INT32_MIN + 1)

Why do you want this to stay negative when 1 is subtracted?

I'm not sure that we actually need to add a "real" namespace here. Rather we could define some sort of limit for what is a valid namespace and then leave it up to the getElementsByTagName code explicitly set the namespace to PR_INT32_MIN or -2 or whatever.

Btw, since you're here, could you remove the dumb code in GetElementsByTagNameNS that checks if the namespace is unresolved yet and returns an explicitly empty list if so? That is both unneccesary code, and wrong since such elements could be inserted later.
> Why do you want this to stay negative when 1 is subtracted?

Because I want |index < 0| to be true if someone passes this to GetNameSpaceURI for some wacko reason.

I'm not sure how what I did is different from what you're suggesting... Could you explain?  Do you just want me to nix kNameSpaceID_Reserved and have all the magic be in nsContentList.h?  I added kNameSpaceID_Reserved so that I could assert things are not it in the namespace manager...

And yes, I can fix GetElementsByTagNameNS.  We should probably write a test for that situation.
(In reply to comment #6)
> > Why do you want this to stay negative when 1 is subtracted?
> 
> Because I want |index < 0| to be true if someone passes this to
> GetNameSpaceURI for some wacko reason.

Couldn't you assert at the top of NameSpaceManagerImpl::GetNameSpaceURI instead? I.e. assert that the id is >= 0.

> I'm not sure how what I did is different from what you're suggesting... Could
> you explain?  Do you just want me to nix kNameSpaceID_Reserved and have all 
> the magic be in nsContentList.h?

Yeah, basically.

> I added kNameSpaceID_Reserved so that I could
> assert things are not it in the namespace manager...

Could you just assert that all ids are >= -1 instead? Or even in some cases >= 0 where kNameSpaceID_Unknown doesn't make any sense.

> And yes, I can fix GetElementsByTagNameNS.  We should probably write a test 
> for that situation.

Yeah. We'd have to use random namespaces to ensure that running the test multiple times works.
Attachment #229404 - Attachment is obsolete: true
Attachment #229876 - Flags: superreview?(bugmail)
Attachment #229876 - Flags: review?(bugmail)
Attachment #229404 - Flags: superreview?(bugmail)
Attachment #229404 - Flags: review?(bugmail)
Comment on attachment 229876 [details] [diff] [review]
Updated to comments

Looks great! Thanks!
Attachment #229876 - Flags: superreview?(bugmail)
Attachment #229876 - Flags: superreview+
Attachment #229876 - Flags: review?(bugmail)
Attachment #229876 - Flags: review+
Fixed on trunk.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.