nsXULElement::GetAttrNameAt is severly broken




14 years ago
10 years ago


(Reporter: sicking, Assigned: sicking)



Firefox Tracking Flags

(Not tracked)



(1 attachment)

When getting attribute-names from the prototype attributes we need to skip the
prototype attributes that are set locally, i.e. in the element itself. However
the current code does this compleatly wrong as far as i can tell.

When asking for attribute N it looks if attribute N is locally and if so steps
to attribute N+1 and loops. However that doesn't take into account if any
attributes before N are set locally. Instead we need to loop through all
prototype attributes until we find the Nth that is not set locally.

Unfortunatly this is slower, but the current behaviour seems just outright
broken. Also i doubt that GetAttrNameAt is used in any performance critical places.
GetAttrNameAt is used in the following places where speed may matter:

1)  style resolution, if an attr selector with a * namespace is used (rare)
2)  nsXULDocument::SynchronizeBroadcastListener
3)  nsXULDocument::OverlayForwardReference::Merge
4)  nsXULContentBuilder::BuildContentFromTemplate
5)  nsXULContentBuilder::SynchronizeUsingTemplate
6)  nsXULTemplateBuilder::AddSimpleRuleBindings
7)  nsXULTemplateBuilder::CompileSimpleRule

I'm not sure how much those last 6 depend on it being fast...
I don't really care about * namespace. I've always know it would be slow and I
think i raised that issue with the style WG before that spec made it past last call.

I should also mention that it'll only be slow when there are local attributes
set. So during the initial overlay-merging things should be fine. Most likly
that is what is saving us now, since the current code will always fail when
there are both local and prototype attributes set.
Flags: blocking1.9a1?
Created attachment 207146 [details] [diff] [review]
Patch to fix

This is optimized for speed at the cost of some duplication. The potentially slow task is to remove duplicated names that occur in both the prototype attributes and the local attributes. The code does this by choosing the larger of the two arrays as 'base array' and uses all the names from that array as is.

To get names from the other (i.e. the smaller of the two) array the code steps through each name in the array and searches for duplicates in the larger array. The result is that we will always do fewer searches in the larger array, rather then potentially more searches in the shorter array.

One result of this is that there is no duplicate checking if either of the arrays are empty.

Similar algorithm is used to calculate the number of attributes.

This also fixes the bad out-of-bounds handling causing bug 321810
Assignee: hyatt → bugmail
Attachment #207146 - Flags: superreview?(bzbarsky)
Attachment #207146 - Flags: review?
Comment on attachment 207146 [details] [diff] [review]
Patch to fix

I assume the duplication you meant is between AttrNameAt and GetSafeAttrNameAt?  Would it really be slower to use the former inside the latter?

>Index: xul/content/src/nsXULElement.cpp
>-#if 0 /* || defined(DEBUG_shaver) || defined(DEBUG_waterson) */

Why remove this logging code?  I guess we haven't used it in a while, but are we sure we never plan to?  I thought we were considering prototypes for HTML too, no?

r+sr=bzbarsky modulo those issues.
Attachment #207146 - Flags: superreview?(bzbarsky)
Attachment #207146 - Flags: superreview+
Attachment #207146 - Flags: review?
Attachment #207146 - Flags: review+
Actually, the duplication i was referring to was inside the implementation of GetAttrNameAt and GetAttrCount. They could both be about half size if we sacrifieced some performance.

There's very little codesize that could be saved in GetSafeAttrNameAt, I think the extra code is worth it. 

If we need logging we can write new logging code then. My bet is that the current logging isn't going to fullfill whatever needs we'd have. As things were the logging was just getting in the way and made the code hard to read.
> There's very little codesize that could be saved in GetSafeAttrNameAt, I think
> the extra code is worth it. 

It wouldn't be big codesize savings, but it would be nice mindprint savings, especially sinds one function uses ATTRS() while the other does not (to do the same exact thing).

One other comment I forgot -- please declare the new function right next to 
GetSafeAttrNameAt, since they're so closely related?  And document the difference in the header?
it'd be a good idea to simplify GetSafeAttrNameAt such that it looks more like AttrNameAt, but i'd rather keep the implementations separate. Want a new bug for that?

I'll move the implemenatations and declarations together and add comments.
> Want a new bug for that?

Checked in, i'll file a new bug on GetSafeAttrNameAt
Last Resolved: 12 years ago
Resolution: --- → FIXED
Flags: blocking1.9a1?


10 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: shrir → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.