481591 - fix problems with content: counter() and counters() serialization

Status: RESOLVED FIXED

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

Description

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

I noticed a bunch of problems with serialization of content:counter() and counters() functions.  Patches to follow.

Comment 1

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

Note that these patches apply on top of the patches in bug 478160.

Comment 2

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

Attachment: patch 1: split eCSSUnit_String into String, Ident, and Families (which gives us quotes around the separator-part of counters())

This splits eCSSUnit_String into String (quoted), Ident (unquoted), and Families (font-family list).  Note that the font-family descriptor is String rather than Families.

(I'd like to eventually change Families into some other data type; there's even an existing bug on that.)

Comment 3

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

Attachment: patch 2: fix computed style serialization to output keywords rather than integers

Comment 4

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

Attachment: patch 3: fix serialization of declarations whose list-style-type part is none

Comment 5

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

Attachment: patch 4: tests

I wrote these tests while looking at the code coverage output in http://people.mozilla.org/~mnandigama/codecoverage/src/layout/style/nsCSSDeclaration.cpp.gcov.html , which led me to the bugs fixed here.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 365616 [details] [diff] [review]
patch 1: split eCSSUnit_String into String, Ident, and Families (which gives us quotes around the separator-part of counters())

Can we add an assert on the unit in the nsCSSFontFaceStyleDecl::GetPropertyValue eCSSFonDesc_Family case, where the comment talks about knowing the unit?

Do we still need to Trim() the quote from the eCSSFontDesc_Family in InsertFontFaceRule?

With the assert added and the Trim() checked, looks good.

Comment 7

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

(In reply to comment #6)
> Do we still need to Trim() the quote from the eCSSFontDesc_Family in
> InsertFontFaceRule?

No, and I don't think we did before this patch either.

Comment 8

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

http://hg.mozilla.org/mozilla-central/rev/3428afb4e233
http://hg.mozilla.org/mozilla-central/rev/3b13a87771fe
http://hg.mozilla.org/mozilla-central/rev/36da7fe92b5f
http://hg.mozilla.org/mozilla-central/rev/ac91e21d53c5