321940 - Polish nsAttrAndChildArray::GetSafeAttrNameAt

Status: RESOLVED WONTFIX

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

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: possible patch

I'm not very convinced that this is a good idea since the rest of the code checks for the existance of an attribute the way that GetSafeAttrNameAt currently does it. So right now it's consistent with the rest of the code.

AttrNameAt doesn't bother with checking for existance of attributes, which is why the code there looks different.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Under what conditions would we be missing an attribute here anyway?  That is, why do we need this check?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

When an attribute is removed we don't move all the children in the buffer, but rather leave a gap between the last attribute and the first child. But we only keep track of how many attribute slots are allocated, not how many are actually used.

Comment 4

[Boris Zbarsky [:bzbarsky]]

In that case, the current impl of nsAttrAndChildArray::AttrNameAt is wrong, no?  It'll sometimes return null and miss out on real attributes...

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

No, we compress the attributes part of the buffer so there are never any empty slots in the middle of the array, just between the attributes and the children.

(Note that AttrNameAt will never return null, even on bad accesses to out-of-bounds indexes. It'll return a pointer to a null value)

Comment 6

[Boris Zbarsky [:bzbarsky]]

I guess I'm trying to understand what comment 0 is really saying.  As long as we start walking at ATTRS() and walk by steps of 1 for AttrCount() steps, we'll hit all our attributes and nothing else, right?  Without a need to check what's in that slot as GetSafeAttrNameAt currently does?

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Well, AttrSlots() is implemented by walking the buffer and checking for the first non-empty slots, so we'd check for empty slots no matter what. However doing it the way that GetSafeAttrNameAt is faster since we'll only check a single slot (the one requested)

Comment 8

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #7)
> Well, AttrSlots() is implemented by walking the buffer and checking for the
> first non-empty slots

What's AttrSlots()?  The code in AttrNameAt uses ATTRS(mImpl).  Is that really slower than what GetSafeAttrNameAt does?

I think I'm starting to see what you meant in general, though.  The problem is that we don't store the number of attrs we have, just the number of slots.  And some number of slots at the end may hold null, but for purposes of GetSafeAttrNameAt we don't care how many.  OK.  Given that, I agree that this change is kinda silly.

Let me know whether you still want review or whether you want to just wontfix?

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sorry, i meant AttrCount().

Yeah, it's the fact that we don't store the real attrcount that calls for the current implementation. Would have been easier to do this over irc, it's good to have you back from vacation :)

Using ATTRS more in general would be a good thing I agree. It'd probably not make a difference for the compiled code, but is easier to read. But I'd want to do it consistently everywhere where we check for empty slots and not just in GetSafeAttrNameAt.

Next time I'm in this class I'll make an effort clean things up (with an eye towards how I did this in nsMappedAttributes).