Bug 130351 – LayoutBox is a terrible name

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

We have LayoutUnit, LayoutPoint, LayoutSize, LayoutRect, and LayoutBox. Spot the odd one out.

Also LayoutBox is an enum, not a "box" in the CSS sense. It needs a better name.

[Simon Fraser (smfr)]

Maybe "CSSBoxType"

[zalan]

(In reply to comment #1)
> Maybe "CSSBoxType"
+1

[Zoltan Horvath]

I'm fine with CSSBoxType. If you change it, don't forget to change LayoutBoxExtent.h too.

[Bem Jones-Bey]

(In reply to comment #3)
> I'm fine with CSSBoxType. If you change it, don't forget to change LayoutBoxExtent.h too.

Hrm. Does the name CSSBoxExtent actually make sense? I'm not sure.

[Zoltan Horvath]

CSSBoxModelType and CSSBoxModelExtent?

[Bem Jones-Bey]

(In reply to comment #5)
> CSSBoxModelType and CSSBoxModelExtent?

LayoutBoxExtent is more related to LayoutUnit than LayoutBox, so I don't think it needs to be renamed.

[Bem Jones-Bey]

Created attachment 227088 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 227088 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227088&action=review

> Source/WebCore/rendering/shapes/Shape.h:72
> -    static PassOwnPtr<Shape> createLayoutBoxShape(const RoundedRect&, WritingMode, Length margin, Length padding);
> +    static PassOwnPtr<Shape> createBoxShape(const RoundedRect&, WritingMode, Length margin, Length padding);

Not sure that "box shape" makes sense here, since you're not passing a CSSBoxType parameter.

> Source/WebCore/rendering/style/RenderStyleConstants.h:559
> +enum CSSBoxType { BoxMissing = 0, MarginBox, BorderBox, PaddingBox, ContentBox, Fill, Stroke, ViewBox };

This patch makes me wonder if we should call this a CSSReferenceBox

[Bem Jones-Bey]

(In reply to comment #8)
> (From update of attachment 227088 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227088&action=review
> 
> > Source/WebCore/rendering/shapes/Shape.h:72
> > -    static PassOwnPtr<Shape> createLayoutBoxShape(const RoundedRect&, WritingMode, Length margin, Length padding);
> > +    static PassOwnPtr<Shape> createBoxShape(const RoundedRect&, WritingMode, Length margin, Length padding);
> 
> Not sure that "box shape" makes sense here, since you're not passing a CSSBoxType parameter.

The problem is that this never should have been called "LayoutBoxShape" in the first place, as it creates a BoxShape object. Also, since the Shape classes don't deal with renderers themselves, it expects the caller (ShapeInfo::computedShape) to look at the BoxTime and create a ReoundedRect with the proper dimensions. 

> 
> > Source/WebCore/rendering/style/RenderStyleConstants.h:559
> > +enum CSSBoxType { BoxMissing = 0, MarginBox, BorderBox, PaddingBox, ContentBox, Fill, Stroke, ViewBox };
> 
> This patch makes me wonder if we should call this a CSSReferenceBox

Strictly speaking, it is only a Reference Box when it is specified with another shape. If the box is specified by itself, then the spec calls it a Box Shape, because in that case, it is the box that specifies the shape. I tried to change the names from layoutBox to cssBox when it isa a box shape, and to referenceBox when it applies to another shape, but there were a couple of places where that wouldn't have been possible without refactoring.

[WebKit Commit Bot]

Comment on attachment 227088 [details]
Patch

Clearing flags on attachment: 227088

Committed r165843: <http://trac.webkit.org/changeset/165843>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.