Bug 128685

Created attachment 224003 [details] Patch

Created attachment 224016 [details] Patch I've decided to put all of my planned renames into this patch. So here they are.

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.

(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?

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.

> > 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. :)

Comment on attachment 224016 [details] Patch Clearing flags on attachment: 224016 Committed r164006: <http://trac.webkit.org/changeset/164006>

All reviewed patches have been landed. Closing bug.