Closed Bug 898455 Opened 7 years ago Closed 7 years ago

domUtils.getCSSValuesForProperty contains duplicate entries


(Core :: CSS Parsing and Computation, defect)

Windows 7
Not set





(Reporter: Optimizer, Assigned: almasry.mina)




(1 file)

if you do domUtils.getCSSValuesForProperty('border') , then you get an array with all the colors present 4 times each and "none" present twice.

This is because the assumption that the array is sorted while inserting the colors is wrong as "inherit" is present before any other color.
So the problem here is that we end up adding colors for each of the four border-color subproperties, but when we add colors we don't do the unique thing....

Mina, I think we should change back to doing the unique thing for colors here.  :(  And maybe add something where we track which parser variant flags we've seen for subproperties so far and skip doing things that would be a subset of those flags, so we don't end up doing the (no-op) insertion of colors 3 extra times here.  Want to take this?
Flags: needinfo?(almasry.mina)
Uh oh. Mina made a bad. Here is a patch that fixes that, and as a bonus adds "url" to the values returned from getCSSValuesForProperty.

I was also thinking of ways to make this faster, but nothing optimal came up. If you want, I could:

- use std::set for this, instead of nsTArray.
- I could add an enumerator on SplayTree, and use it here instead of nsTArray
- I could use an nsTHashtable<nsStringHashKey> 

I didn't do any of these things yet because I wanted to check with you first. But if you want any of these changes, here is a patch.
Attachment #781953 - Flags: review?(bzbarsky)
Flags: needinfo?(almasry.mina)
Comment on attachment 781953 [details] [diff] [review]

Yeah, this works.  r=me
Attachment #781953 - Flags: review?(bzbarsky) → review+
Alrighty then.
Keywords: checkin-needed
Assignee: nobody → almasry.mina
Keywords: checkin-needed
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.