Bug 130351

Summary: LayoutBox is a terrible name
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Layout and RenderingAssignee: Bem Jones-Bey <bjonesbe>
Status: RESOLVED FIXED    
Severity: Normal CC: bjonesbe, commit-queue, dino, dstockwell, esprehn+autocc, glenn, gyuyoung.kim, kondapallykalyan, krit, macpherson, menard, simon.fraser, zalan, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 127982    
Attachments:
Description Flags
Patch none

Simon Fraser (smfr)
Reported 2014-03-17 10:59:00 PDT
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.
Attachments
Patch (25.95 KB, patch)
2014-03-18 12:30 PDT, Bem Jones-Bey 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Simon Fraser (smfr)
Comment 1 2014-03-17 11:25:41 PDT
Maybe "CSSBoxType"
alan
Comment 2 2014-03-17 11:56:10 PDT
(In reply to comment #1) > Maybe "CSSBoxType" +1
Zoltan Horvath
Comment 3 2014-03-17 14:58:35 PDT
I'm fine with CSSBoxType. If you change it, don't forget to change LayoutBoxExtent.h too.
Bem Jones-Bey
Comment 4 2014-03-17 15:04:10 PDT
(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
Comment 5 2014-03-17 15:26:52 PDT
CSSBoxModelType and CSSBoxModelExtent?
Bem Jones-Bey
Comment 6 2014-03-17 15:53:47 PDT
(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
Comment 7 2014-03-18 12:30:13 PDT
Created attachment 227088 [details] Patch
Simon Fraser (smfr)
Comment 8 2014-03-18 12:35:54 PDT
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
Comment 9 2014-03-18 13:25:38 PDT
(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 10 2014-03-18 13:59:06 PDT
Comment on attachment 227088 [details] Patch Clearing flags on attachment: 227088 Committed r165843: <http://trac.webkit.org/changeset/165843>
WebKit Commit Bot
Comment 11 2014-03-18 13:59:11 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.