475214 - CSS serialization uses "-x-system-font"

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect, P4)

Description

[Jesse Ruderman]

Attachment: testcase

The parser won't accept -x-system-font, so the serializer shouldn't emit it.

Comment 1

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

I'm guessing the behavior changed with bug 376075, but I'm also guessing it wasn't that great before either.

Comment 2

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

Attachment: patch

New test passes, whereas it failed 3 of 8 (the second and fourth serialization tests, and the fourth getter test) without the patch.

Full layout/style mochitests pass.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 360658 [details] [diff] [review]
patch

Looks ok, but since no one looks at the return value of GetValueOrImportantValue, do we want to just make it return |const nsCSSValue&| so you don't have to do the comma operator thing here?

Comment 4

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

Returning |const nsCSSValue&| rather than |nsCSSValue| doesn't make much sense unless we're returning the same thing that's input (which is dangerous, see 420069) or we just stop checking for the branch that currently has an NS_ERROR in it.  I guess I'd lean towards the latter, but I also wouldn't want to do that as part of this patch; it's not a small change.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yeah, I was assuming we'd return the value input...

Let's not worry about it; I'd forgotten the PGO thing.  :(

Comment 6

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

Hmm, actually, I also need to check consistence of !important in the ToString code here.

Comment 7

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

Attachment: patch

Here's a revised patch that also checks consistency of importance, and adds four additional tests, one of which the old patch failed.

It also doesn't use GetValueOrImportantValue.

Comment 8

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

Attachment: remove GetValueOrImportantValue

And here's a patch to remove GetValueOrImportantValue entirely.

Comment 9

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

Attachment: typesafe wrappers for nsCSSDataBlock::StorageFor

... and make the replacement for GetValueOrImportantValue a little less ugly.

(The replacement is also faster since the caller always knows the desired importance, and the replacement depends on that whereas GetValueOrImportantValue figured it out.)

Comment 10

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

Note also that a few more of these typesafe wrappers end up used in my multiple-backgrounds patch (bug 322475).

Comment 11

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

http://hg.mozilla.org/mozilla-central/rev/768c8c967b0d
http://hg.mozilla.org/mozilla-central/rev/461bf98a6426
http://hg.mozilla.org/mozilla-central/rev/cfc21a608dcd