898455 - domUtils.getCSSValuesForProperty contains duplicate entries

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Girish Sharma [:Optimizer]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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?

Comment 2

[Mina Almasry]

Attachment: css-auto-fix.patch

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 781953 [details] [diff] [review]
css-auto-fix.patch

Yeah, this works.  r=me

Comment 4

[Mina Almasry]

Alrighty then.

Comment 5

[Jacek Caban]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4fae10b52148

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4fae10b52148