645620 - Remove nsCSSStructs

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P4)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

After bug 636039 I was planning to remove the nsCSS* structs, though I was thinking of making bigger changes to nsCSSExpandedDataBlock (maybe or maybe not along the lines of bug 553456).

However, I realized how *trivial* it is to remove them right now (thanks largely to bug 576044) -- so we should do that, since it lets us remove code we don't need and simplifies the process of adding new CSS properties.

Patch queue coming up in a bit -- patch 1 is the "stop using them" patch, and the rest of the patches are removing things we no longer need as a result.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1: switch from nsCSS* to an array of nsCSSValue

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2: remove nsCSSExpandedDataBlock::kOffsetTable

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3: move nsCSSCornerSizes to nsCSSValue.h/cpp

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 4: remove nsCSSStruct.h/cpp

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 5: remove the datastruct_ and member_ fields of nsCSSPropList

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 522299 [details] [diff] [review]
patch 1: switch from nsCSS* to an array of nsCSSValue

r=me

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 522300 [details] [diff] [review]
patch 2: remove nsCSSExpandedDataBlock::kOffsetTable

r=me

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 522301 [details] [diff] [review]
patch 3: move nsCSSCornerSizes to nsCSSValue.h/cpp

r=me

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 522302 [details] [diff] [review]
patch 4: remove nsCSSStruct.h/cpp

r=me

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 522303 [details] [diff] [review]
patch 5: remove the datastruct_ and member_ fields of nsCSSPropList

r=me (though I semi-skimmed most of the list there).

Comment 11

[Zack Weinberg (:zwol)]

It's nice to see all this go away (less boilerplate for new CSS properties, yay)

It occurs to me that a lot of the cost of having two nsCSSExpandedDataBlock objects embedded in nsCSSParser (and therefore a lot of the motivation for bug 553456) is the cost of running the nsCSSValue constructor and destructor for every value slot.  What do you think of doing just-in-time initialization on value slots?  This would be quite easy now we have an array instead of a bunch of structs -- replace the array with a char[] storage block, and PropertyAt() uses placement new to initialize the appropriate slot if it hasn't already been done (as determined by the bitfields).  I can try to code that up later today if you like the idea.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Well, I'd like to move towards getting rid of nsCSSExpandedDataBlock entirely -- so I think such a patch would be temporary.  In other words, it seems reasonable if it's really simple.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/mozilla-central/rev/0864cd7f9e9b
https://hg.mozilla.org/mozilla-central/rev/967f25420b71
https://hg.mozilla.org/mozilla-central/rev/ebb436e2d8d0
https://hg.mozilla.org/mozilla-central/rev/e76c7e9b17cf
https://hg.mozilla.org/mozilla-central/rev/0aec17daf6f1