Bug 95625

Created attachment 161791 [details] Patch

*** Bug 95490 has been marked as a duplicate of this bug. ***

Attachment 161791 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1 Source/WebCore/rendering/WrapShapeInfo.cpp:123: One space before end of line comments [whitespace/comments] [5] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 161830 [details] Patch Removed the extra space check-webkit-style was complaining about.

Comment on attachment 161830 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161830&action=review Overall I'm left scratching my head here regarding the mixture of physical and logical terminology. Your platform/graphics additions seem to be using largely physical terminology, but then everything you're doing with them over in the rendering code is in terms of logical coordinates (i.e., line top and line bottom are x values in vertical writing-modes, but you're happily passing those values in to your XShape code, which then calls them "y"). I think you need to have a clear design intent here. Either the shapes are physical, in which case you need to be doing a translation at the WrapShapeInfo level, or the shapes themselves are logical, in which case I don't think you should be using x and y terminology in their implementations. I'm also wondering what the reasoning is behind putting all of the classes into platform/graphics. Especially if they end up being logical, I'm not sure there is any reason to have them over there instead of just putting them in the rendering directory. > Source/WebCore/ChangeLog:8 > + Initial commit of a subset of the Xhapes classes. Typo, should be XShapes. > Source/WebCore/ChangeLog:10 > + This set of classes enables the exclusions layout code to determine how to break up a line into segements Typo, segements should be segments. > Source/WebCore/ChangeLog:11 > + that will fit within or around a shape, given the Y coordinates of the line's top edge and bottom edges. Do you mean logical top rather than Y? > Source/WebCore/platform/graphics/XSRectangle.h:39 > +class XSRectangle : public XShape { I think a comment above this class declaration describing what it represents might be helpful here. > Source/WebCore/platform/graphics/XSRectangle.h:54 > + virtual FloatRect shapeBoundingBox() const { return FloatRect(m_x, m_y, m_width, m_height); } > + virtual void getOutsideIntervals(float y1, float y2, Vector<XSInterval>&) const; > + virtual void getInsideIntervals(float y1, float y2, Vector<XSInterval>&) const; You're missing OVERRIDEs on these three functions > Source/WebCore/platform/graphics/XSRectangle.h:62 > + float m_rx; > + float m_ry; I would add a comment here explaining what these are. A person who isn't familiar with this code will see x,y,width,height and understand those, but may be left scratching their head wondering what m_rx and m_ry are here. > Source/WebCore/platform/graphics/XShape.h:43 > + static PassOwnPtr<XShape> createXShape(const BasicShape*, float borderBoxWidth, float borderBoxHeight); borderBoxWidth and borderBoxHeight are physical terminology. Is this physical or logical? I'm confused about this. > Source/WebCore/rendering/WrapShapeInfo.cpp:110 > + m_xshape = XShape::createXShape(shape, logicalWidth, logicalHeight); Here you're passing in logical values to a function whose (see my comment above) arguments had physical names. Which is correct? > Source/WebCore/rendering/WrapShapeInfo.h:70 > + LayoutUnit shapeTop() const Is this logical or physical? If it's supposed to be a value that is compared with a line top or bottom, then this should be renamed to shapeLogicalTop(). > Source/WebCore/rendering/WrapShapeInfo.h:103 > + FloatRect shapeBounds = m_xshape->shapeBoundingBox(); This appears to be logical. Is that the intent?

Oh, I mentioned this on IRC, but I would just put the XShape classes in rendering/ and rename them to ExclusionShape, ExclusionRectangle, etc. I don't think these classes are going to end up being anything generically reusable, so they should just be closer to the Exclusion implementation. Is there any particular reason why WrapShape info isn't just called ExclusionShapeInfo?

(In reply to comment #6) Dave, The intent is to use Shapes both with exclusions and with floats (and eventually with masks or anything else that needs a shape). Do you have any suggestions on how to make these classes generically reusable?

Created attachment 162390 [details] Patch Just the XS => ExclusionShape relocation/rename. Moved the classes from platform/graphics to rendering and renamed the classes: XSInterval => ExclusionInterval XSRectangle = >ExclusionRectangle XSShape => ExclusionShape This patch is just to shakeout rename related build problems on the EWS bots.

Attachment 162390 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1 Source/WebCore/rendering/WrapShapeInfo.h:37: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 162408 [details] Patch Fixed the style-checker problem (alphabetized WrapInfo include files, and update the XCode project.

Created attachment 162542 [details] One shape-outside exclusion shape overlaps elements with different writing-modes.

(In reply to comment #5) > (From update of attachment 161830 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161830&action=review > > Overall I'm left scratching my head here regarding the mixture of physical and logical terminology. Your platform/graphics additions seem to be using largely physical terminology, but then everything you're doing with them over in the rendering code is in terms of logical coordinates (i.e., line top and line bottom are x values in vertical writing-modes, but you're happily passing those values in to your XShape code, which then calls them "y"). Here's a proposal that I hope addresses the problem you've brought up. I think the ExclusionShape API should use logical coordinates for the incoming top/bottom line edges and similarly it should report line segments' start/stops with logical X coordinates. The incoming BasicShape, which is just an encoding of the CSS shape, is defined with physical coordinates. That means that createExclusionShape() method will need an additional parameter, so that it can transform the BasicShape's physical coordinates when the writing-mode is vertical. It also means that an ExclusionShape will be specific to a writing-mode, so in a shape-outside case like this: https://bug-95625-attachments.webkit.org/attachment.cgi?id=162542, two ExclusionShape objects will be used to compute the layouts. Internally, the ExclusionShapes classes (just ExclusionRectangle for the moment) do their computations as if the writing-mode was horizontal. I don't think this needs to change, although the ExclusionInterval class, which is used to report line segment endpoints, should use logical names for the X coordinates, start and end instead of x1 and x2. Here's what the ExclusionShape API would become: // A representation of a BasicShape that enables layout code to determine how to break a line up into segments // that will fit within or around a shape. The line is defined by a pair of logical Y coordinates and the // computed segments are returned as pairs of logical X coordinates (ExclusionIntervals). class ExclusionShape { public: static PassOwnPtr<ExclusionShape> createExclusionShape(const BasicShape*, float borderBoxWidth, float borderBoxHeight, WritingMode); virtual ~ExclusionShape() { } virtual LayouRect shapeBoundingBox() const = 0; virtual void getInsideIntervals(float logicalTop, float logicalBottom, Vector<ExclusionInterval>&) const = 0; virtual void getOutsideIntervals(float logicalTop, float logicalBottom, Vector<ExclusionInterval>&) const = 0; }; } // namespace WebCore The shapeBoundingBox() would return the logical bounds of the exclusion shape. I've changed the return type from FloatRect to LayoutRect. Currently the exclusion layout code only supports shape-inside (so an exclusion shape only affects one element's contents) and it explicitly ignores exclusions defined for vertical content (see WrapShapeInfo::isWrapShapeInfoEnabledForRenderBlock). If possible, I'd like to file a bug about the lack of writing-mode support in ExclusionShape, rather than making the changes now.

(In reply to comment #6) > Oh, I mentioned this on IRC, but I would just put the XShape classes in rendering/ and rename them to ExclusionShape, ExclusionRectangle, etc. I don't think these classes are going to end up being anything generically reusable, so they should just be closer to the Exclusion implementation. I have moved and renamed the XS classes. > Is there any particular reason why WrapShape info isn't just called ExclusionShapeInfo? The current name is (now) largely historical, and I'd be happy to change it. OK, if I file a separate bug for that?

(In reply to comment #5) > (From update of attachment 161830 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161830&action=review > > Source/WebCore/ChangeLog:8 > > + Initial commit of a subset of the Xhapes classes. > > Typo, should be XShapes. > > > Source/WebCore/ChangeLog:10 > > + This set of classes enables the exclusions layout code to determine how to break up a line into segements > > Typo, segements should be segments. I have fixed these typos. > > Source/WebCore/platform/graphics/XSRectangle.h:54 > > + virtual FloatRect shapeBoundingBox() const { return FloatRect(m_x, m_y, m_width, m_height); } > > + virtual void getOutsideIntervals(float y1, float y2, Vector<XSInterval>&) const; > > + virtual void getInsideIntervals(float y1, float y2, Vector<XSInterval>&) const; > > You're missing OVERRIDEs on these three functions Added the OVERRIDES. > > > Source/WebCore/platform/graphics/XSRectangle.h:62 > > + float m_rx; > > + float m_ry; > > I would add a comment here explaining what these are. A person who isn't familiar with this code will see x,y,width,height and understand those, but may be left scratching their head wondering what m_rx and m_ry are here. Added comments.

(In reply to comment #5) > (From update of attachment 161830 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161830&action=review If, per comment #10, it's OK to make the changes vis logical vs physical geometry in a separate bug fix, I'd like to address the following then: > Do you mean logical top rather than Y? > > > Source/WebCore/platform/graphics/XSRectangle.h:39 > > +class XSRectangle : public XShape { > > I think a comment above this class declaration describing what it represents might be helpful here. [I've proposed such a comment in #10. Since it depends on resolving logical vs physical geometry, I'd like to include with that TBD patch] > > Source/WebCore/platform/graphics/XShape.h:43 > > + static PassOwnPtr<XShape> createXShape(const BasicShape*, float borderBoxWidth, float borderBoxHeight); > > borderBoxWidth and borderBoxHeight are physical terminology. Is this physical or logical? I'm confused about this. > > > Source/WebCore/rendering/WrapShapeInfo.cpp:110 > > + m_xshape = XShape::createXShape(shape, logicalWidth, logicalHeight); > > Here you're passing in logical values to a function whose (see my comment above) arguments had physical names. Which is correct? > > > Source/WebCore/rendering/WrapShapeInfo.h:70 > > + LayoutUnit shapeTop() const > > Is this logical or physical? If it's supposed to be a value that is compared with a line top or bottom, then this should be renamed to shapeLogicalTop(). > > > Source/WebCore/rendering/WrapShapeInfo.h:103 > > + FloatRect shapeBounds = m_xshape->shapeBoundingBox(); > > This appears to be logical. Is that the intent?

Comment on attachment 162408 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162408&action=review Looks pretty good. Just minor comments. > Source/WebCore/ChangeLog:499 > - * platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp: > + * rendering/harfbuzz/FontPlatformDataHarfBuzz.cpp: Not sure what this is about? I don't see this code in the patch. > Source/WebCore/ChangeLog:504 > - * platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.h: > + * rendering/harfbuzz/FontPlatformDataHarfBuzz.h: Ditto. > Source/WebCore/rendering/ExclusionShape.h:43 > + static PassOwnPtr<ExclusionShape> createExclusionShape(const BasicShape*, float borderBoxWidth, float borderBoxHeight); Rename borderBoxWidth and borderBoxHeight to borderBoxLogicalWidth and borderBoxLogicalHeight, since right now at least, your shapes are clearly logical. > Source/WebCore/rendering/WrapShapeInfo.cpp:110 > + m_xshape = ExclusionShape::createExclusionShape(shape, logicalWidth, logicalHeight); I'd just drop the x here. At this point, inside this class, it's pretty obvious what we're dealing with, so I would just call this m_shape. > Source/WebCore/rendering/WrapShapeInfo.h:70 > + LayoutUnit shapeTop() const Rename this to shapeLogicalTop(), since right now at least, it's clearly logical. > Source/WebCore/rendering/WrapShapeInfo.h:103 > + FloatRect shapeBounds = m_xshape->shapeBoundingBox(); Let's call the local shapeLogicalBoundingBox to help clarify it is logical in nature.

One more comment: virtual void getInsideIntervals(float logicalTop, float logicalBottom, Vector<ExclusionInterval>&) const = 0; virtual void getOutsideIntervals(float logicalTop, float logicalBottom, Vector<ExclusionInterval>&) const = 0; You should go ahead and do those renames to logicalTop and logicalBottom in the headers at least, instead of still using y1 and y2.

I mean minY / maxY.

Created attachment 162938 [details] Patch I've made the suggested changes.

Comment on attachment 162938 [details] Patch I've created the following bugs per the additional work noted previously: ExclusionShape API should use logical coordinates for input/output - https://bugs.webkit.org/show_bug.cgi?id=96156 Rename WrapShapeInfo to ExclusionShapeInfo - https://bugs.webkit.org/show_bug.cgi?id=96157

Comment on attachment 162938 [details] Patch r=me

Comment on attachment 162938 [details] Patch Clearing flags on attachment: 162938 Committed r128083: <http://trac.webkit.org/changeset/128083>

All reviewed patches have been landed. Closing bug.