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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
12 years ago
2 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 1 obsolete attachment)

958 bytes, patch
Details | Diff | Splinter Review
8.40 KB, patch
Details | Diff | Splinter Review
11.16 KB, patch
Details | Diff | Splinter Review
8.56 KB, patch
Details | Diff | Splinter Review
6.99 KB, patch
Details | Diff | Splinter Review
56.71 KB, patch
Details | Diff | Splinter Review
675 bytes, patch
Details | Diff | Splinter Review
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.)
Flags: in-testsuite?
No longer depends on: 160403
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).
Duplicate of this bug: 442630
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Created attachment 353170 [details] [diff] [review]
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.
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.
Created attachment 354077 [details] [diff] [review]
patch 1: remove an old comment
Attachment #354077 - Flags: superreview?(bzbarsky)
Attachment #354077 - Flags: review?(bzbarsky)
Created attachment 354078 [details] [diff] [review]
patch 2: check consistency before deciding we can return the 'border' shorthand
Attachment #354078 - Flags: superreview?(bzbarsky)
Attachment #354078 - Flags: review?(bzbarsky)
Created attachment 354079 [details] [diff] [review]
patch 3: suppress -moz-use-text-color in shorthands

Port this fix from the serialization code to the property getters.
Attachment #354079 - Flags: superreview?(bzbarsky)
Attachment #354079 - Flags: review?(bzbarsky)
Created attachment 354080 [details] [diff] [review]
patch 4: condense four-side shorthands to minimal values

Port another fix from serialization code to getters.
Attachment #354080 - Flags: superreview?(bzbarsky)
Attachment #354080 - Flags: review?(bzbarsky)
Created attachment 354081 [details] [diff] [review]
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'.)
Attachment #354081 - Flags: superreview?(bzbarsky)
Attachment #354081 - Flags: review?(bzbarsky)
Created attachment 354082 [details] [diff] [review]
patch 6: rewrite serialization (nsCSSDeclaration::ToString) to use GetValue

This is the main patch; an update of attachment 353170 [details] [diff] [review].
Attachment #353170 - Attachment is obsolete: true
Attachment #354082 - Flags: superreview?(bzbarsky)
Attachment #354082 - Flags: review?(bzbarsky)
Attachment #354077 - Flags: superreview?(bzbarsky)
Attachment #354077 - Flags: superreview+
Attachment #354077 - Flags: review?(bzbarsky)
Attachment #354077 - Flags: 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?
Attachment #354078 - Flags: superreview?(bzbarsky)
Attachment #354078 - Flags: superreview+
Attachment #354078 - Flags: review?(bzbarsky)
Attachment #354078 - Flags: review+
Attachment #354079 - Flags: superreview?(bzbarsky)
Attachment #354079 - Flags: superreview+
Attachment #354079 - Flags: review?(bzbarsky)
Attachment #354079 - Flags: review+
Attachment #354080 - Flags: superreview?(bzbarsky)
Attachment #354080 - Flags: superreview+
Attachment #354080 - Flags: review?(bzbarsky)
Attachment #354080 - Flags: review+
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.  :(
Attachment #354081 - Flags: superreview?(bzbarsky)
Attachment #354081 - Flags: superreview+
Attachment #354081 - Flags: review?(bzbarsky)
Attachment #354081 - Flags: review+
(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!
Attachment #354082 - Flags: superreview?(bzbarsky)
Attachment #354082 - Flags: superreview+
Attachment #354082 - Flags: review?(bzbarsky)
Attachment #354082 - Flags: review+
(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
Blocks: 377731
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
> 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.)
Created attachment 357920 [details] [diff] [review]
fix delete that should have been delete[]

Fix a mistake in patch 6, pointed out by valgrind.
Attachment #357920 - Flags: superreview?(bzbarsky)
Attachment #357920 - Flags: review?(bzbarsky)
Attachment #357920 - Flags: superreview?(bzbarsky)
Attachment #357920 - Flags: superreview+
Attachment #357920 - Flags: review?(bzbarsky)
Attachment #357920 - Flags: review+
Duplicate of this bug: 188972
You need to log in before you can comment on or make changes to this bug.