Bug 128685 – [CSS Shapes] Rename shapeSize and others to make ShapeInfo and friends easier to understand

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

[Bem Jones-Bey]

Every time I look at these classes, I get confused. This set of renames is the beginning of reducing that confusion. There will be some more renames coming in a future patch.

[Bem Jones-Bey]

Created attachment 224003 [details]
Patch

[Bem Jones-Bey]

Created attachment 224016 [details]
Patch

I've decided to put all of my planned renames into this patch. So here they are.

[Zoltan Horvath]

I like most of the changes except this. 

m_shapeLineTop is always greater or equal than the shape's top position, and less or equal than the shape's bottom position, so the naming "referenceBoxLineTop" a bit confusing for me.

[Bem Jones-Bey]

(In reply to comment #3)
> I like most of the changes except this. 
> 
> m_shapeLineTop is always greater or equal than the shape's top position, and less or equal than the shape's bottom position, so the naming "referenceBoxLineTop" a bit confusing for me.

I think you're incorrect there. In both shape-inside and shape-outside, m_referenceBoxLineTop is initialized to the coordinates of the line in the reference box coordinate system. (see ShapeInsideInfo::updateSegmentsForLine, ShapeOutsideInfo::updateDeltasForContainingBlockLine). This can be less than the shape's top position or greater than the shape's bottom position.

In the shape-inside case, this can get updated in ShapeInsideInfo::adjustLogicalLineTop to be a position inside of the shape. However, note that this position is still relative to the reference box top, not relative to the top of the shape. (because the coordinates of the shape itself are relative to the reference box.)

Does that make sense? If you think that isn't correct, can you explain to me what is incorrect about my understanding?

[Zoltan Horvath]

Comment on attachment 224016 [details]
Patch

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

(In reply to comment #4)
> (In reply to comment #3)
> > I like most of the changes except this. 
> > 
> > m_shapeLineTop is always greater or equal than the shape's top position, and less or equal than the shape's bottom position, so the naming "referenceBoxLineTop" a bit confusing for me.
> 
> I think you're incorrect there. In both shape-inside and shape-outside, m_referenceBoxLineTop is initialized to the coordinates of the line in the reference box coordinate system. (see ShapeInsideInfo::updateSegmentsForLine, ShapeOutsideInfo::updateDeltasForContainingBlockLine). This can be less than the shape's top position or greater than the shape's bottom position.
>
> In the shape-inside case, this can get updated in ShapeInsideInfo::adjustLogicalLineTop to be a position inside of the shape. However, note that this position is still relative to the reference box top, not relative to the top of the shape. (because the coordinates of the shape itself are relative to the reference box.)

Yes, in the case of shape-inside, it's always updated by ShapeInsideInfo::adjustLogicalLineTop. I forget to include that to my comment, sorry! Also I didn't think about the members usecase for shape-outside.
 
> Does that make sense? If you think that isn't correct, can you explain to me what is incorrect about my understanding?

From the point of shape-outside I totally agree with the naming. However, I think for shape-inside the current naming makes more sense, I don't really have an argument against naming it in a more general way, since it's relative to the ref. box.

I set the cq+ for you!

> Source/WebCore/rendering/shapes/ShapeInfo.h:205
> +    LayoutUnit m_referenceBoxLineTop;

I like most of the changes except this. 

m_shapeLineTop is always greater or equal than the shape's top position, and less or equal than the shape's bottom position, so the naming "referenceBoxLineTop" a bit confusing for me.

[Zoltan Horvath]

> > Source/WebCore/rendering/shapes/ShapeInfo.h:205
> > +    LayoutUnit m_referenceBoxLineTop;
> 
> I like most of the changes except this. 
> 
> m_shapeLineTop is always greater or equal than the shape's top position, and less or equal than the shape's bottom position, so the naming "referenceBoxLineTop" a bit confusing for me.

Ignore this, the reply got confused. :)

[WebKit Commit Bot]

Comment on attachment 224016 [details]
Patch

Clearing flags on attachment: 224016

Committed r164006: <http://trac.webkit.org/changeset/164006>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.