376075 - nsCSSDeclaration::ToString puts inherit / -moz-initial in the middle of shorthands

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

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

nsCSSDeclaration::ToString puts 'inherit' and '-moz-initial' values in the middle of shorthands, which is not allowed.  All callers of nsCSSDeclaration::AllPropertiesSameImportance also need to check if any of the values are 'initial' or 'inherit'.  They should probably then set the shorthand to whichever (inherit, initial, or other) has the largest number, and then let the rest be specified separately.  (We could, in fact, do something similar with !important, except the shorthand would always have to be the not-!important ones.)

Comment 1

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

I think we should perhaps fix this (especially after bug 160403 is fixed) by making nsCSSDeclaration::ToString share more code with the other serialization methods, and by making it detect that shorthands can be condensed in a more systematic way (by looping over subproperty tables).

Comment 3

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

Attachment: work in progress

The main problems with this work in progress that I know about are that it regresses a few things where the property getter code was inferior to the serialization code, in particular:
 * shortening of shorthand properties, particularly those with box sides
 * omitting of -moz-use-text-color in shorthands

The former causes 2 mochitest failures; both need more tests, as noted in the XXX comments in test_value_storage.html within this patch.

Comment 4

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

I'm going to attach a stack of patches (somewhat deep) that should complete the fix here.

The first group of patches are a set of fixes for bugs in the getter code, which would lead to regressions if we switch serialization to using the getter code if they were not fixed first.  (There's a bunch of patches because there are a bunch of small fixes; I've kept them separate although in some cases I fixed multiple occurrences of the same problem in one patch.)  Each of these patches has tests that exercise both the serialization and getter code (except for the last one, where the tests test only the getter code).

Then the final patch will be an updated version of attachment 353170 [details] [diff] [review] to switch the serialization code to use the getter code.

Comment 5

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

Attachment: patch 1: remove an old comment

Comment 6

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

Attachment: patch 2: check consistency before deciding we can return the 'border' shorthand

Comment 7

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

Attachment: patch 3: suppress -moz-use-text-color in shorthands

Port this fix from the serialization code to the property getters.

Comment 8

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

Attachment: patch 4: condense four-side shorthands to minimal values

Port another fix from serialization code to getters.

Comment 9

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

Attachment: patch 5: fix the 'font' and 'background' getters to be more conservative

Fix the 'font' and 'background' getters to check that the unspecifiable subproperties aren't non-default values.  (Note that bug 322475 gets rid of having unspecifiable subproperties for 'background'.)

Comment 10

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

Attachment: patch 6: rewrite serialization (nsCSSDeclaration::ToString) to use GetValue

This is the main patch; an update of attachment 353170 [details] [diff] [review].

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 354078 [details] [diff] [review]
patch 2: check consistency before deciding we can return the 'border' shorthand

Looks good, but would it make sense to make subproptables static?  Or can we assume the compiler will optimize things ok here?  Or just not care?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 354080 [details] [diff] [review]
patch 4: condense four-side shorthands to minimal values

Looks good.  I think we might have even had a separate bug on this..

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 354081 [details] [diff] [review]
patch 5: fix the 'font' and 'background' getters to be more conservative

Looks good, though I keep wishing we had default values in nsCSSPropList somehow.  :(

Comment 14

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

(In reply to comment #11)
> (From update of attachment 354078 [details] [diff] [review])
> Looks good, but would it make sense to make subproptables static?  Or can we
> assume the compiler will optimize things ok here?  Or just not care?

That would require both:
 * exporting private stuff from nsCSSProps
 * paying a slight penalty in library load time (or maybe first-run-through-the-code) on some platforms to deal with relocating the base address
... so I don't think it's worth it.

And I think it's probably optimized well enough as-is, and I don't care.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 354082 [details] [diff] [review]
patch 6: rewrite serialization (nsCSSDeclaration::ToString) to use GetValue

> (STILL NEED TO FIX BACKGROUND AND FONT SHORTHANDS FOR PROPERTIES THEY CAN'T REPRESENT)

That's done, right?

>+++ b/layout/style/nsCSSDeclaration.cpp
> nsCSSDeclaration::AppendPropertyAndValueToString(nsCSSProperty aProperty,
>+  NS_ASSERTION((aProperty < eCSSProperty_COUNT_no_shorthands) ==
>+                 aValue.IsEmpty(),
>+               "aValue should be given for non-shorthands but not shorthands");

The string should be the other way around, right?  aValue should be given for shorthands.

>+++ b/layout/style/nsCSSProps.cpp
>+nsCSSProps::BuildShorthandsContainingTable() 

How much would it slow startup to go through at the end of this method, ifdef DEBUG, and check for every longhand that all its containing shorthands have it in their longhand tables, and conversely that for every shorthand every prop in its longhand table has it as a containing shorthand?  It'd be awfully nice to be able to have such assertions.

>+++ b/layout/style/nsCSSProps.h
>+  static nsCSSProperty* gShorthandsContainingPool;

Document the structure of that array, maybe?

It's nice to see all that old code going away!

Comment 16

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

(In reply to comment #15)
> (From update of attachment 354082 [details] [diff] [review])
> > (STILL NEED TO FIX BACKGROUND AND FONT SHORTHANDS FOR PROPERTIES THEY CAN'T REPRESENT)
> 
> That's done, right?

Yes, that was attachment 354081 [details] [diff] [review].

> The string should be the other way around, right?  aValue should be given for
> shorthands.

Yep.

> How much would it slow startup to go through at the end of this method, ifdef
> DEBUG, and check for every longhand that all its containing shorthands have it
> in their longhand tables, and conversely that for every shorthand every prop in
> its longhand table has it as a containing shorthand?  It'd be awfully nice to
> be able to have such assertions.

Done.

> Document the structure of that array, maybe?

Done.  (If I understood you correctly.)


Fixed on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/5d0f70073bb6
http://hg.mozilla.org/mozilla-central/rev/d696886de032
http://hg.mozilla.org/mozilla-central/rev/db84bf8e7438
http://hg.mozilla.org/mozilla-central/rev/4acaa6ee3665
http://hg.mozilla.org/mozilla-central/rev/15b55742e0cd
http://hg.mozilla.org/mozilla-central/rev/0ed096f666ec

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Done.  (If I understood you correctly.)

Looks like you did, yes.

Comment 18

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

I landed this along with some other patches, but it looks like it was about 5K codesize reduction on Linux and 8K on Mac.  (I think it's almost entirely the patches in this bug.)

Comment 19

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

Attachment: fix delete that should have been delete[]

Fix a mistake in patch 6, pointed out by valgrind.

Comment 20

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

http://hg.mozilla.org/mozilla-central/rev/5e57cb13ae9b