Bug 83380

Created attachment 136054 [details] Proposed clean-up.

I vote to rename the computedPadding* functions to cssOnlyPadding*. In all the padding functions something is "computed". I know you mean this in the sense of "computed CSS", but I don't know if a casual reader would get it. > // The content area of the box (excludes padding - and intrinsic padding for table cells - and border). // The content area of the box (excludes padding - and intrinsic padding for table cells, etc. - and border). > // This returns the CSS computed padding values. // These return the CSS computed padding values. > // Those functions are used during layout. Table cells override > // them to include an alignment padding (called intrinsic padding). These functions are used during layout. Table cells and some MathML renderers override them to include extra "intrinsic padding" for alignment and other uses. I need paddingStart and paddingEnd to be virtual also, for MathML. === With the above changes, I really like this patch! For the record, here's my point of view. I'd like to use intrinsic padding for MathML also, as in my recent patch for bug 82353, where intrinsic padding leaves room for a square root sign. I like to think of intrinsic padding as maybe "decorated padding" that's similar to a fancy kind of border, but contained in the "padding". This would be very helpful for MathML. That is, when RenderBlock::layout() uses the equation border-box = content-box + padding + border (+ maybe scroll bars), I want it to allow this padding to be more than just padding from CSS styles. Then this "rendering padding" needs to be treated as padding by availableLogicalWidth() for example when computing space for laying out or centering children. So RenderBoxModelObject is a base class with two kinds of padding functions. The rendering one that includes intrinsic padding is virtual, so derived classes like TableCell and some MathML classes can override it. Other classes don't know the specifics of intrinsic padding in these derived classes, but they do know there are two kinds of padding functions. Actually the one without intrinsic padding is mostly just used by computed styles, and doesn't need to be virtual, as you point out.

Comment on attachment 136054 [details] Proposed clean-up. Yes seems like a better solution and less intrusive.

(In reply to comment #2) > I vote to rename the computedPadding* functions to cssOnlyPadding*. In all the padding functions something is "computed". I know you mean this in the sense of "computed CSS", but I don't know if a casual reader would get it. For the record, I thought about a similar name (cssPadding*) but I thought this would be confusing as it's not the raw CSS information (ie a Length) but a computed CSS information (percent / calculated length have been resolved). computedCSSPadding* could be a good compromise here.

computedCSSPadding* sounds great to me.

Created attachment 136562 [details] Updated patch against ToT, also took Dave's comment into account.

Comment on attachment 136562 [details] Updated patch against ToT, also took Dave's comment into account. LGTM, FWIW.

Comment on attachment 136562 [details] Updated patch against ToT, also took Dave's comment into account. Seems reasonable to me.

Comment on attachment 136562 [details] Updated patch against ToT, also took Dave's comment into account. Clearing flags on attachment: 136562 Committed r113823: <http://trac.webkit.org/changeset/113823>

All reviewed patches have been landed. Closing bug.