379741 - We have eStyleUnit_Null nsStyleCoords for margin, etc.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

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

We have eStyleUnit_Null nsStyleCoords for a bunch of CSS properties where the null duplicates some other initial value that is representable in nsStyleCoord.  We should instead represent these initial values so we don't have to worry about two different nsStyleCoord values representing the same thing.

This also causes some bugs in the computed style code:  we show (since some recent refactoring that bz did, I think) 'none' as the computed value for margin, padding, etc. properties on elements that don't have frames.

Comment 1

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

Attachment: patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

So you can remove the check for eStyleUnit_Null from IsFixedPaddingSize() and IsFixedMarginSize() in nsAbsoluteContainingBlock.cpp, right?  Or better yet expose IsFixedUnit from nsStyleStruct.cpp and make this code use that (and remove the check for _Null in that as well)?

Also, can the line-height be eStyleUnit_Null at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsLineLayout.cpp&rev=3.261#2012 ?  Or can that check be removed too?

Same for the check in NonZeroStyleCoord() in nsContainerFrame.cpp (only used for border-radius).

Same for NonZeroStyleCoord() in nsDisplayList.cpp.

Similar for IsPaddingZero() and IsMarginZero() in nsInlineFrame.cpp.

Similar for functions with the same names in nsBlockFrame.cpp

r+sr=bzbarsky for the patch as-is if you want to do the cleanups in a different bug.

Followup: Do we want to avoid duplicating IsAutoHeight() between nsLayoutUtils and nsFrame.cpp?

Comment 3

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

I'm thinking I should eventually move to where GetUnit() can
  NS_ASSERTION(mUnit != eStyleUnit_Null, "reading uninitialized value");

Comment 4

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

Attachment: add eStyleUnit_None and use it for max-width/height

This removes the remaining use of null units to mean anything other than uninitialized.

Comment 5

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

Attachment: remove the remaining eStyelUnit_Null checks

This removes the remaining eStyleUnit_Null checks (mostly obsoleted by attachment 263767 [details] [diff] [review]; some unneeded before it) and makes GetUnit assert that mUnit is not null.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 264423 [details] [diff] [review]
add eStyleUnit_None and use it for max-width/height

r+sr=bzbarsky

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 264424 [details] [diff] [review]
remove the remaining eStyelUnit_Null checks

r+sr=bzbarsky

Comment 8

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

All 3 patches checked in to trunk.

Comment 9

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

One of these patches (probably the first) also fixed the marker-offset:auto failure in test_value_computation.html.