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.)

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).

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.

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.

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

Port another fix from serialization code to getters.

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'.)

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

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 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 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. :(

(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 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!

(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

> Done. (If I understood you correctly.) Looks like you did, yes.

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.)

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