Bug 128685 - [CSS Shapes] Rename shapeSize and others to make ShapeInfo and friends easier to understand
Summary: [CSS Shapes] Rename shapeSize and others to make ShapeInfo and friends easier...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Bem Jones-Bey
URL:
Keywords:
Depends on:
Blocks: 128631
  Show dependency treegraph
 
Reported: 2014-02-12 13:49 PST by Bem Jones-Bey
Modified: 2014-02-12 18:57 PST (History)
5 users (show)

See Also:


Attachments
Patch (18.39 KB, patch)
2014-02-12 13:57 PST, Bem Jones-Bey
no flags Details | Formatted Diff | Diff
Patch (23.01 KB, patch)
2014-02-12 15:22 PST, Bem Jones-Bey
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bem Jones-Bey 2014-02-12 13:49:13 PST
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.
Comment 1 Bem Jones-Bey 2014-02-12 13:57:23 PST
Created attachment 224003 [details]
Patch
Comment 2 Bem Jones-Bey 2014-02-12 15:22:06 PST
Created attachment 224016 [details]
Patch

I've decided to put all of my planned renames into this patch. So here they are.
Comment 3 Zoltan Horvath 2014-02-12 16:01:39 PST
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.
Comment 4 Bem Jones-Bey 2014-02-12 17:16:57 PST
(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 5 Zoltan Horvath 2014-02-12 18:26:49 PST
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.
Comment 6 Zoltan Horvath 2014-02-12 18:29:08 PST
> > 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 7 WebKit Commit Bot 2014-02-12 18:57:49 PST
Comment on attachment 224016 [details]
Patch

Clearing flags on attachment: 224016

Committed r164006: <http://trac.webkit.org/changeset/164006>
Comment 8 WebKit Commit Bot 2014-02-12 18:57:51 PST
All reviewed patches have been landed.  Closing bug.