Bug 96156

Summary:

[CSS Exclusions] ExclusionShape API should use logical coordinates for input/output

Product:

WebKit

Reporter:

Hans Muller <giles_joplin>

Component:

CSS

Assignee:

Hans Muller <giles_joplin>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

betravis, donggwan.kim, eric, gyuyoung.kim, krit, rakuco, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

89705, 95625

Bug Blocks:

89707

Attachments:

Description	Flags
Diagram: conversion to ExclusionShape internal coordinates, vertical-lr writing-mode	none
Diagram: conversion to ExclusionShape internal coordinates, vertical-rl writing-mode	none
Patch	none
Patch	none
Patch	krit: review-, krit: commit-queue-
Patch	none
Patch	krit: review-, krit: commit-queue-
Patch	none
Attached to incorrect bug	none

Hans Muller

Reported 2012-09-07 16:40:25 PDT

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 borderBoxLogicalWidth, float borderBoxLogicalHeight, WritingMode); virtual ~ExclusionShape() { } virtual LayouRect shapeLogicalBoundingBox() 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 shapeLogicalBoundingBox() would return the logical bounds of the exclusion shape. I've changed the return type from FloatRect to LayoutRect.

Attachments
Diagram: conversion to ExclusionShape internal coordinates, vertical-lr writing-mode (545.14 KB, image/png) 2012-09-14 10:19 PDT, Hans Muller	no flags	Details
Diagram: conversion to ExclusionShape internal coordinates, vertical-rl writing-mode (556.32 KB, image/png) 2012-09-14 10:20 PDT, Hans Muller	no flags	Details
Patch (15.41 KB, patch) 2012-09-14 10:29 PDT, Hans Muller	no flags	Details Formatted Diff Diff
Patch (22.18 KB, patch) 2012-09-17 11:42 PDT, Hans Muller	no flags	Details Formatted Diff Diff
Patch (25.18 KB, patch) 2012-09-18 11:50 PDT, Hans Muller	krit: review- krit: commit-queue-	Details Formatted Diff Diff
Patch (19.86 KB, patch) 2012-09-21 12:34 PDT, Hans Muller	no flags	Details Formatted Diff Diff
Patch (22.33 KB, patch) 2012-09-21 16:47 PDT, Hans Muller	krit: review- krit: commit-queue-	Details Formatted Diff Diff
Patch (22.08 KB, patch) 2012-09-24 11:06 PDT, Hans Muller	no flags	Details Formatted Diff Diff
Attached to incorrect bug (41.07 KB, patch) 2012-09-24 14:47 PDT, Bear Travis	no flags	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Hans Muller

Comment 1 2012-09-14 10:19:46 PDT

Created attachment 164180 [details] Diagram: conversion to ExclusionShape internal coordinates, vertical-lr writing-mode

Hans Muller

Comment 2 2012-09-14 10:20:40 PDT

Created attachment 164182 [details] Diagram: conversion to ExclusionShape internal coordinates, vertical-rl writing-mode

Hans Muller

Comment 3 2012-09-14 10:24:42 PDT

Internally, ExclusionShapes are oriented so that lines run horizontally. BasicShapes define shape-inside or shape-outside shapes in physcial coordinates, so no transformation is needed when the corresponding element's writing-mode is horizontal. For the two vertical writing-modes, the BasicShape and the logical top,bottom line edge parameters must be transformed. The transformation is trivial and is illustrated in the two attached diagrams.

Hans Muller

Comment 4 2012-09-14 10:29:25 PDT

Created attachment 164185 [details] Patch

Bear Travis

Comment 5 2012-09-14 11:42:48 PDT

Comment on attachment 164185 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164185&action=review Looking good. Two high-level questions: Is it worth turning on vertical layout in wrap shapes, so we can write some simple test cases? And would it be worth it to convert to using completely logical coordinates internally? Right now, it seems like they are logical but not flipped, in the case of right to left vertical layout. > Source/WebCore/ChangeLog:12 > + system when the writing-mode is vertical. Similarly, the getInclude,ExcludedIntervals() Is it worth describing the internal coordinate system in the changelog? It seems to be a non-flipped logical coordinate system. > Source/WebCore/rendering/ExclusionShape.h:41 > +struct LineSegment { There seems to be a little bit of a naming disconnect between RenderBlock's "LineSegment" and ExclusionShape's "ExclusionInterval". With methods the getIncluded/ExcludedIntervals, it seems like it would make more sense to keep ExclusionInterval here. On the RenderBlock side, LineSegment will eventually have to contain more information than just the start and end coordinates, so it may make sense to keep them separate.

Bear Travis

Comment 6 2012-09-14 11:56:11 PDT

Comment on attachment 164185 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164185&action=review > Source/WebCore/rendering/WrapShapeInfo.cpp:123 > + Extra newline

Hans Muller

Comment 7 2012-09-17 11:42:51 PDT

Created attachment 164426 [details] Patch Correct how ExclusionShapes deal with logical coordinates and enable shape-inside exclusion layout for vertical writing-modes.

Hans Muller

Comment 8 2012-09-18 11:50:00 PDT

Created attachment 164598 [details] Patch

Hans Muller

Comment 9 2012-09-18 12:01:18 PDT

(In reply to comment #8) > Created an attachment (id=164598) [details] Updated the patch to reflect the fix for https://bugs.webkit.org/show_bug.cgi?id=93547, which just landed this morning.

Dirk Schulze

Comment 10 2012-09-21 01:22:27 PDT

Comment on attachment 164598 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164598&action=review > LayoutTests/fast/exclusions/shape-inside/shape-inside-vertical-text-expected.html:2 > +<!DOCTYPE html> > +<html> The ChangeLog for the layout tests is missing. > Source/WebCore/ChangeLog:809 > +2012-09-14 Hans Muller <hmuller@adobe.com> > + > + ExclusionShape API should use logical coordinates for input/output > + https://bugs.webkit.org/show_bug.cgi?id=96156 > + oops. Merge failure? > Source/WebCore/ChangeLog:846 > + (WebCore::LineSegment::LineSegment): relocated (for now) from WrapShapeInfo.h for now? Where does it belong to? > Source/WebCore/ChangeLog:2115 > +2012-09-14 Hans Muller <hmuller@adobe.com> > + > + ExclusionShape API should use logical coordinates for input/output > + https://bugs.webkit.org/show_bug.cgi?id=96156 Ditto? > Source/WebCore/rendering/ExclusionRectangle.cpp:43 > +void ExclusionRectangle::getExcludedIntervals(float logicalTop, float logicalBottom, SegmentList& result) const s/logicalTop/localTop/ As far as I understood your changelog, these values are in relative coordinates. In this case we use local (for local coordinate space). Ditto for all other names. > Source/WebCore/rendering/ExclusionRectangle.cpp:46 > + float y1 = minYForLogicalLine(logicalTop, logicalBottom); > + float y2 = maxYForLogicalLine(logicalTop, logicalBottom); Why is this not needed for x1 and x2? > Source/WebCore/rendering/ExclusionRectangle.h:54 > + virtual void getExcludedIntervals(float logicalTop, float logicalBottom, SegmentList&) const OVERRIDE; > + virtual void getIncludedIntervals(float logicalTop, float logicalBottom, SegmentList&) const OVERRIDE; Ditto. s/logical/local/ > Source/WebCore/rendering/ExclusionShape.cpp:62 > +// If the writingMode is vertical, then the BasicShape's (physical) x and y coordinates are swapped, so that Do you have another name for physical? Maybe absolute? Sounds better. absolute is also used on other places for this context. > Source/WebCore/rendering/ExclusionShape.cpp:67 > - if (!wrapShape) > + if (!basicShape) Will this just be used for BasicShape? Will it be used for SVG shapes as well (if we get them in the future)? If so, I would rename them to shape, or exclusionShape. > Source/WebCore/rendering/ExclusionShape.cpp:73 > + OwnPtr<ExclusionShape> exclusionShape; so exclusionShape is already used :) > Source/WebCore/rendering/ExclusionShape.cpp:82 > + float x = floatValueForLength(rectangle->x(), boxWidth); > + float y = floatValueForLength(rectangle->y(), boxHeight); > + float width = floatValueForLength(rectangle->width(), boxWidth); > + float height = floatValueForLength(rectangle->height(), boxHeight); Can you combine this to a FloatRect? > Source/WebCore/rendering/ExclusionShape.cpp:86 > + float radiusX = radiusXLength.isUndefined() ? 0 : floatValueForLength(radiusXLength, boxWidth); > + float radiusY = radiusYLength.isUndefined() ? 0 : floatValueForLength(radiusYLength, boxHeight); FloatSize? > Source/WebCore/rendering/ExclusionShape.cpp:90 > + ? createExclusionRectangle(x, y, width, height, radiusX, radiusY) > + : createExclusionRectangle(y, x, height, width, radiusY, radiusX); Would be great if it can take a FloatRect(), FloatSize(). This is how we usually handle these cases in WebKit. Only exclusions are JS API's. > Source/WebCore/rendering/ExclusionShape.cpp:97 > + float centerX = floatValueForLength(circle->centerX(), boxWidth); > + float centerY = floatValueForLength(circle->centerY(), boxHeight); FloatPoint > Source/WebCore/rendering/ExclusionShape.cpp:102 > + ? createExclusionCircle(centerX, centerY, radius) > + : createExclusionCircle(centerY, centerX, radius); createExclusionCircle(FloatPoint, float)? > Source/WebCore/rendering/ExclusionShape.cpp:109 > + float centerX = floatValueForLength(ellipse->centerX(), boxWidth); > + float centerY = floatValueForLength(ellipse->centerY(), boxHeight); FloatPoint > Source/WebCore/rendering/ExclusionShape.cpp:111 > + float radiusX = floatValueForLength(ellipse->radiusX(), boxWidth); > + float radiusY = floatValueForLength(ellipse->radiusY(), boxHeight); FloatSize > Source/WebCore/rendering/ExclusionShape.cpp:115 > + ? createExclusionEllipse(centerX, centerY, radiusX, radiusY) > + : createExclusionEllipse(centerY, centerX, radiusY, radiusX); createExclusionEllipse(FloatPoint, FloatSize) > Source/WebCore/rendering/ExclusionShape.cpp:127 > + exclusionShape->m_logicalBoxWidth = logicalBoxWidth; > + exclusionShape->m_logicalBoxHeight = logicalBoxHeight; exclusionShape->localSize(FloatSize())? > Source/WebCore/rendering/ExclusionShape.h:49 > + float logicalLeft; > + float logicalRight; > + > + LineSegment(float logicalLeft, float logicalRight) > + : logicalLeft(logicalLeft) > + , logicalRight(logicalRight) > + { > + } Shouldn't a LineSegment have two points? Are these lines just horizontal? > Source/WebCore/rendering/ExclusionShape.h:62 > + static PassOwnPtr<ExclusionShape> createExclusionShape(const BasicShape*, float logicalBoxWidth, float logicalBoxHeight, WritingMode); FloatSize() > Source/WebCore/rendering/ExclusionShape.h:68 > + virtual void getIncludedIntervals(float logicalTop, float logicalBottom, SegmentList&) const = 0; > + virtual void getExcludedIntervals(float logicalTop, float logicalBottom, SegmentList&) const = 0; FloatPoint > Source/WebCore/rendering/ExclusionShape.h:78 > + float m_logicalBoxWidth; > + float m_logicalBoxHeight; It would look better if you use FloatSize here instead.

Hans Muller

Comment 11 2012-09-21 10:00:07 PDT

(In reply to comment #10) > (From update of attachment 164598 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=164598&action=review > > > LayoutTests/fast/exclusions/shape-inside/shape-inside-vertical-text-expected.html:2 > > +<!DOCTYPE html> > > +<html> > > The ChangeLog for the layout tests is missing. Yes it is, I'll fix that. > > Source/WebCore/ChangeLog:809 > > +2012-09-14 Hans Muller <hmuller@adobe.com> > > + > > + ExclusionShape API should use logical coordinates for input/output > > + https://bugs.webkit.org/show_bug.cgi?id=96156 > > + > > oops. Merge failure? Yes! Will fix that. > > > Source/WebCore/ChangeLog:846 > > + (WebCore::LineSegment::LineSegment): relocated (for now) from WrapShapeInfo.h > > for now? Where does it belong to? I'll leave the parenthetical statement out of the ChangeLog. In the future, if additional layout-specific data is added to the LineSegment type, then we may relocate it. > > Source/WebCore/ChangeLog:2115 > > +2012-09-14 Hans Muller <hmuller@adobe.com> > > + > > + ExclusionShape API should use logical coordinates for input/output > > + https://bugs.webkit.org/show_bug.cgi?id=96156 > > Ditto? Yes. I'm going to reset the ChangeLog and then re-insert my changes at the top. Note sure why the usual auto-merge ChangeLog stuff wasn't working for me. > > Source/WebCore/rendering/ExclusionRectangle.cpp:43 > > +void ExclusionRectangle::getExcludedIntervals(float logicalTop, float logicalBottom, SegmentList& result) const > > s/logicalTop/localTop/ As far as I understood your changelog, these values are in relative coordinates. In this case we use local (for local coordinate space). > ... > Ditto for all other names. The patch uses the term "logical coordinates" in the same way that the layout code does: for values that correspond to physical X or Y coordinates, depending on the writing-mode. The origin also moves, depending on the writing-mode. When the writing-mode is "vertical-rl" the origin moves to the top right of the content box's physical location. You can see the implications of this in the two diagrams I attached to the bug. They show how logical coordinates are converted to the ExclusionShape "internal" coordinates. These "internal" coordinates simplify the implementations of the ExclusionShape getIncludedIntervals() and getExcludedIntervals() methods (particularly for the polygon case, which hasn't been included yet) as well the algorithm described here for finding the 'first fit' line segment location - http://hansmuller-webkit.blogspot.com/2012/08/revised-algorithm-for-finding-first.html. Lines are always horizontal in internal coordinates: they're defined by two increasing Y values. Similarly internal line segments are always defined by a pair of increasing X values. The term "physical coordinates" is the one used by the layout system to refer to the usual graphics coordinate system. BasicShapes are defined in physical coordinates. > > Source/WebCore/rendering/ExclusionRectangle.cpp:46 > > + float y1 = minYForLogicalLine(logicalTop, logicalBottom); > > + float y2 = maxYForLogicalLine(logicalTop, logicalBottom); > > Why is this not needed for x1 and x2? Because none of the writing-modes affect our internal X axis. > > Source/WebCore/rendering/ExclusionRectangle.h:54 > > + virtual void getExcludedIntervals(float logicalTop, float logicalBottom, SegmentList&) const OVERRIDE; > > + virtual void getIncludedIntervals(float logicalTop, float logicalBottom, SegmentList&) const OVERRIDE; > > Ditto. s/logical/local/ See above. > > Source/WebCore/rendering/ExclusionShape.cpp:62 > > +// If the writingMode is vertical, then the BasicShape's (physical) x and y coordinates are swapped, so that > > Do you have another name for physical? Maybe absolute? Sounds better. absolute is also used on other places for this context. The phsycial/logical distinction is mentioned here: http://trac.webkit.org/wiki/April%202012%20Write%20Your%20Own%20Render%20Object It was the principal reason for creating this patch, see https://bugs.webkit.org/show_bug.cgi?id=95625#c5. > > Source/WebCore/rendering/ExclusionShape.cpp:67 > > - if (!wrapShape) > > + if (!basicShape) > > Will this just be used for BasicShape? Will it be used for SVG shapes as well (if we get them in the future)? If so, I would rename them to shape, or exclusionShape. All of the exclusion code is likely to evolve a great deal over the next couple of months. I'd be happy to substitute a better name for basicShape when the scope of the implementation warrants it. > > Source/WebCore/rendering/ExclusionShape.cpp:82 > > + float x = floatValueForLength(rectangle->x(), boxWidth); > > + float y = floatValueForLength(rectangle->y(), boxHeight); > > + float width = floatValueForLength(rectangle->width(), boxWidth); > > + float height = floatValueForLength(rectangle->height(), boxHeight); > > Can you combine this to a FloatRect? > > Source/WebCore/rendering/ExclusionShape.cpp:86 > > + float radiusX = radiusXLength.isUndefined() ? 0 : floatValueForLength(radiusXLength, boxWidth); > > + float radiusY = radiusYLength.isUndefined() ? 0 : floatValueForLength(radiusYLength, boxHeight); > > FloatSize? > > Source/WebCore/rendering/ExclusionShape.cpp:90 > > + ? createExclusionRectangle(x, y, width, height, radiusX, radiusY) > > + : createExclusionRectangle(y, x, height, width, radiusY, radiusX); > > Would be great if it can take a FloatRect(), FloatSize(). This is how we usually handle these cases in WebKit. Only exclusions are JS API's. I don't see the advantage of packing and unpacking these values for a purely internal function call. Why is this: ... { float x = ... ; float y = ... ; float width = ... ; float height = ... ; localFunction(FloatRect(x, y, width height)) } localFunction(FloatRect rect) { anotherFunction(rect.x, rect.y, rect.width, rect.height); } better than this: ... { float x = ... ; float y = ... ; float width = ... ; float height = ... ; localFunction(x, y, width, height); } localFunction(float x, float y, float width, float height) anotherFunction(x, y, width, height); } > > Source/WebCore/rendering/ExclusionShape.h:49 > > + float logicalLeft; > > + float logicalRight; > > + > > + LineSegment(float logicalLeft, float logicalRight) > > + : logicalLeft(logicalLeft) > > + , logicalRight(logicalRight) > > + { > > + } > > Shouldn't a LineSegment have two points? Are these lines just horizontal? Yes, LineSegments are horizontal lines. > > Source/WebCore/rendering/ExclusionShape.h:68 > > + virtual void getIncludedIntervals(float logicalTop, float logicalBottom, SegmentList&) const = 0; > > + virtual void getExcludedIntervals(float logicalTop, float logicalBottom, SegmentList&) const = 0; > > FloatPoint The names of these arguments reflect the structure of the caller, which also keeps the parameters separate, as does the line layout code. The names were specified in the review for https://bugs.webkit.org/show_bug.cgi?id=95625.

Dirk Schulze

Comment 12 2012-09-21 12:29:16 PDT

(In reply to comment #11) > > s/logicalTop/localTop/ As far as I understood your changelog, these values are in relative coordinates. In this case we use local (for local coordinate space). > > ... > > Ditto for all other names. > > The patch uses the term "logical coordinates" in the same way that the layout code does: for values that correspond to physical X or Y coordinates, depending on the writing-mode. The origin also moves, depending on the writing-mode. When the writing-mode is "vertical-rl" the origin moves to the top right of the content box's physical location. You can see the implications of this in the two diagrams I attached to the bug. They show how logical coordinates are converted to the ExclusionShape "internal" coordinates. These "internal" coordinates simplify the implementations of the ExclusionShape getIncludedIntervals() and getExcludedIntervals() methods (particularly for the polygon case, which hasn't been included yet) as well the algorithm described here for finding the 'first fit' line segment location - http://hansmuller-webkit.blogspot.com/2012/08/revised-algorithm-for-finding-first.html. Lines are always horizontal in internal coordinates: they're defined by two increasing Y values. Similarly internal line segments are always defined by a pair of increasing X values. > > The term "physical coordinates" is the one used by the layout system to refer to the usual graphics coordinate system. BasicShapes are defined in physical coordinates. ... > The phsycial/logical distinction is mentioned here: http://trac.webkit.org/wiki/April%202012%20Write%20Your%20Own%20Render%20Object Sounds reasonable. Didn't know of the usage in layout code. > > > > Source/WebCore/rendering/ExclusionRectangle.cpp:46 > > > + float y1 = minYForLogicalLine(logicalTop, logicalBottom); > > > + float y2 = maxYForLogicalLine(logicalTop, logicalBottom); > > > > Why is this not needed for x1 and x2? > > Because none of the writing-modes affect our internal X axis. Ok. > > > Source/WebCore/rendering/ExclusionShape.cpp:67 > > > - if (!wrapShape) > > > + if (!basicShape) > > > > Will this just be used for BasicShape? Will it be used for SVG shapes as well (if we get them in the future)? If so, I would rename them to shape, or exclusionShape. K. > > All of the exclusion code is likely to evolve a great deal over the next couple of months. I'd be happy to substitute a better name for basicShape when the scope of the implementation warrants it. > > > > Source/WebCore/rendering/ExclusionShape.cpp:82 > > > + float x = floatValueForLength(rectangle->x(), boxWidth); > > > + float y = floatValueForLength(rectangle->y(), boxHeight); > > > + float width = floatValueForLength(rectangle->width(), boxWidth); > > > + float height = floatValueForLength(rectangle->height(), boxHeight); > > > > Can you combine this to a FloatRect? > > > > Source/WebCore/rendering/ExclusionShape.cpp:86 > > > + float radiusX = radiusXLength.isUndefined() ? 0 : floatValueForLength(radiusXLength, boxWidth); > > > + float radiusY = radiusYLength.isUndefined() ? 0 : floatValueForLength(radiusYLength, boxHeight); > > > > FloatSize? > > > > Source/WebCore/rendering/ExclusionShape.cpp:90 > > > + ? createExclusionRectangle(x, y, width, height, radiusX, radiusY) > > > + : createExclusionRectangle(y, x, height, width, radiusY, radiusX); > > > > Would be great if it can take a FloatRect(), FloatSize(). This is how we usually handle these cases in WebKit. Only exclusions are JS API's. > > I don't see the advantage of packing and unpacking these values for a purely internal function call. Why is this: > > ... > { > float x = ... ; > float y = ... ; > float width = ... ; > float height = ... ; > localFunction(FloatRect(x, y, width height)) > } > > localFunction(FloatRect rect) { > anotherFunction(rect.x, rect.y, rect.width, rect.height); > } No, you wouldn't use the variables first. you would do something like localFunction(FloatRect(..., ..., ..., ...)) > > > better than this: > > ... > { > > float x = ... ; > float y = ... ; > float width = ... ; > float height = ... ; > localFunction(x, y, width, height); > } > > localFunction(float x, float y, float width, float height) > anotherFunction(x, y, width, height); > } localFunction(const FloatRect& rect) { anotherFunction(rect); } You just need to pass the reference, not 4 floats. Since .x(), .y(), .width(), .height() are inline, you don't have performance lost. > > > > Source/WebCore/rendering/ExclusionShape.h:49 > > > + float logicalLeft; > > > + float logicalRight; > > > + > > > + LineSegment(float logicalLeft, float logicalRight) > > > + : logicalLeft(logicalLeft) > > > + , logicalRight(logicalRight) > > > + { > > > + } > > > > Shouldn't a LineSegment have two points? Are these lines just horizontal? > > Yes, LineSegments are horizontal lines. Can you find a better name? Maybe HorizontalSegment? However, don't have strong feelings about that. > > > > Source/WebCore/rendering/ExclusionShape.h:68 > > > + virtual void getIncludedIntervals(float logicalTop, float logicalBottom, SegmentList&) const = 0; > > > + virtual void getExcludedIntervals(float logicalTop, float logicalBottom, SegmentList&) const = 0; > > > > FloatPoint > > The names of these arguments reflect the structure of the caller, which also keeps the parameters separate, as does the line layout code. The names were specified in the review for https://bugs.webkit.org/show_bug.cgi?id=95625. For top, bottom you wouldn't create a FloatPoint. Just seems to be logical incorrect, sorry. But for top,right or width, height or cx,cy or rx,ry please use FloatPoint/FloatSize.

Hans Muller

Comment 13 2012-09-21 12:34:05 PDT

Created attachment 165172 [details] Patch

Hans Muller

Comment 14 2012-09-21 16:47:20 PDT

Created attachment 165225 [details] Patch This version of the patch eschews individual ExclusionShape.cpp parameters for FloatRects, FloatSizes, and FloatPoints.

Dirk Schulze

Comment 15 2012-09-24 08:33:58 PDT

Comment on attachment 165225 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165225&action=review Looks good in general, just some snippets. > LayoutTests/ChangeLog:3522 > - > + merge error again :) > Source/WebCore/rendering/ExclusionRectangle.h:49 > + , m_x(bounds.x()) > + , m_y(bounds.y()) > + , m_width(bounds.width()) > + , m_height(bounds.height()) > + , m_rx(radii.width()) > + , m_ry(radii.height()) Do the m_x, m_y... need to be floats? Can't you take FloatRect m_boundingBox, FloatSize m_radius? > Source/WebCore/rendering/ExclusionShape.cpp:54 > + return adoptPtr(new ExclusionRectangle(FloatRect(center.x() - radius, center.y() - radius, center.x() + radius, center.y() + radius), FloatSize(radius, radius))); There is and offset function in FloatSize IIRC. Can you check please? > Source/WebCore/rendering/ExclusionShape.cpp:83 > + float x = floatValueForLength(rectangle->x(), boxWidth); > + float y = floatValueForLength(rectangle->y(), boxHeight); > + float width = floatValueForLength(rectangle->width(), boxWidth); > + float height = floatValueForLength(rectangle->height(), boxHeight); Why is that not a FloatRect? > Source/WebCore/rendering/ExclusionShape.cpp:85 > + Length radiusXLength = rectangle->cornerRadiusX(); > + Length radiusYLength = rectangle->cornerRadiusY(); ...and FloatSize? > Source/WebCore/rendering/ExclusionShape.cpp:87 > + float radiusX = radiusXLength.isUndefined() ? 0 : floatValueForLength(radiusXLength, boxWidth); > + float radiusY = radiusYLength.isUndefined() ? 0 : floatValueForLength(radiusYLength, boxHeight); Ditto. > Source/WebCore/rendering/ExclusionShape.cpp:91 > + ? createExclusionRectangle(FloatRect(x, y, width, height), FloatSize(radiusX, radiusY)) > + : createExclusionRectangle(FloatRect(y, x, height, width), FloatSize(radiusY, radiusX)); Creating these objects here would be unnecessary then. > Source/WebCore/rendering/ExclusionShape.cpp:98 > + float centerX = floatValueForLength(circle->centerX(), boxWidth); > + float centerY = floatValueForLength(circle->centerY(), boxHeight); Ditto. Just waste of memory allocation. > Source/WebCore/rendering/ExclusionShape.cpp:112 > + float centerX = floatValueForLength(ellipse->centerX(), boxWidth); > + float centerY = floatValueForLength(ellipse->centerY(), boxHeight); > + float radiusX = floatValueForLength(ellipse->radiusX(), boxWidth); > + float radiusY = floatValueForLength(ellipse->radiusY(), boxHeight); Ditto.

Dirk Schulze

Comment 16 2012-09-24 09:21:04 PDT

(In reply to comment #15) > (From update of attachment 165225 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165225&action=review > > Looks good in general, just some snippets. > > > LayoutTests/ChangeLog:3522 > > - > > + > > merge error again :) > > > Source/WebCore/rendering/ExclusionRectangle.h:49 > > + , m_x(bounds.x()) > > + , m_y(bounds.y()) > > + , m_width(bounds.width()) > > + , m_height(bounds.height()) > > + , m_rx(radii.width()) > > + , m_ry(radii.height()) > > Do the m_x, m_y... need to be floats? Can't you take FloatRect m_boundingBox, FloatSize m_radius? > > > Source/WebCore/rendering/ExclusionShape.cpp:54 > > + return adoptPtr(new ExclusionRectangle(FloatRect(center.x() - radius, center.y() - radius, center.x() + radius, center.y() + radius), FloatSize(radius, radius))); > > There is and offset function in FloatSize IIRC. Can you check please? > > > Source/WebCore/rendering/ExclusionShape.cpp:83 > > + float x = floatValueForLength(rectangle->x(), boxWidth); > > + float y = floatValueForLength(rectangle->y(), boxHeight); > > + float width = floatValueForLength(rectangle->width(), boxWidth); > > + float height = floatValueForLength(rectangle->height(), boxHeight); > > Why is that not a FloatRect? > > > Source/WebCore/rendering/ExclusionShape.cpp:85 > > + Length radiusXLength = rectangle->cornerRadiusX(); > > + Length radiusYLength = rectangle->cornerRadiusY(); > > ...and FloatSize? I am mistaken on the Length values, these can not be FloatSize. Need to check why they are Length at all.(commuting right now) > > > Source/WebCore/rendering/ExclusionShape.cpp:87 > > + float radiusX = radiusXLength.isUndefined() ? 0 : floatValueForLength(radiusXLength, boxWidth); > > + float radiusY = radiusYLength.isUndefined() ? 0 : floatValueForLength(radiusYLength, boxHeight); > > Ditto. > > > Source/WebCore/rendering/ExclusionShape.cpp:91 > > + ? createExclusionRectangle(FloatRect(x, y, width, height), FloatSize(radiusX, radiusY)) > > + : createExclusionRectangle(FloatRect(y, x, height, width), FloatSize(radiusY, radiusX)); > > Creating these objects here would be unnecessary then. > > > Source/WebCore/rendering/ExclusionShape.cpp:98 > > + float centerX = floatValueForLength(circle->centerX(), boxWidth); > > + float centerY = floatValueForLength(circle->centerY(), boxHeight); > > Ditto. Just waste of memory allocation. > > > Source/WebCore/rendering/ExclusionShape.cpp:112 > > + float centerX = floatValueForLength(ellipse->centerX(), boxWidth); > > + float centerY = floatValueForLength(ellipse->centerY(), boxHeight); > > + float radiusX = floatValueForLength(ellipse->radiusX(), boxWidth); > > + float radiusY = floatValueForLength(ellipse->radiusY(), boxHeight); > > Ditto.

Hans Muller

Comment 17 2012-09-24 10:45:38 PDT

(In reply to comment #15) > (From update of attachment 165225 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165225&action=review > > Source/WebCore/rendering/ExclusionRectangle.h:49 > > + , m_x(bounds.x()) > > + , m_y(bounds.y()) > > + , m_width(bounds.width()) > > + , m_height(bounds.height()) > > + , m_rx(radii.width()) > > + , m_ry(radii.height()) > > Do the m_x, m_y... need to be floats? Can't you take FloatRect m_boundingBox, FloatSize m_radius? Doing so would make the ExclusionRectangle implementation more difficult to read and I don't think there's any performance speed/size advantage to using struct valued members here. > > Source/WebCore/rendering/ExclusionShape.cpp:54 > > + return adoptPtr(new ExclusionRectangle(FloatRect(center.x() - radius, center.y() - radius, center.x() + radius, center.y() + radius), FloatSize(radius, radius))); > > There is and offset function in FloatSize IIRC. Can you check please? Yes there are FloatSize mutators however using them would either require making the FloatSize center parameter non-const. That would be a little misleading, since we're only doing so to make it possible to hack the parameter (usually not a good idea). And are expresions like center.expand(-radius) really more readable than what's here? Sadly, what's really wrong here are the width and height ExclusionRectangle parameters for both circles and ellipses. I've fixed that. > > Source/WebCore/rendering/ExclusionShape.cpp:83 > > + float x = floatValueForLength(rectangle->x(), boxWidth); > > + float y = floatValueForLength(rectangle->y(), boxHeight); > > + float width = floatValueForLength(rectangle->width(), boxWidth); > > + float height = floatValueForLength(rectangle->height(), boxHeight); > > Why is that not a FloatRect? > > > Source/WebCore/rendering/ExclusionShape.cpp:85 > > + Length radiusXLength = rectangle->cornerRadiusX(); > > + Length radiusYLength = rectangle->cornerRadiusY(); > > ...and FloatSize? I used coordinate variables here, and elsewhere, because I think what I've got is easier to read. When the x's and y's are implicit, because they're positional FloatRect, FloatSize, it's harder to tell what's going on. For example in this version, it's harder to see that what's happening is that I'm simply swapping x's and y's: exclusionShape = horizontalWritingMode ? createExclusionRectangle( FloatRect( floatValueForLength(rectangle->x(), boxWidth), floatValueForLength(rectangle->y(), boxHeight), floatValueForLength(rectangle->width(), boxWidth), floatValueForLength(rectangle->height(), boxHeight)) FloatSize(radiusX, radiusY)); : createExclusionRectangle( FloatRect( floatValueForLength(rectangle->y(), boxHeight), floatValueForLength(rectangle->x(), boxWidth), floatValueForLength(rectangle->height(), boxHeight)) floatValueForLength(rectangle->width(), boxWidth), FloatSize(radiusY, radiusX)); Here's the original version: exclusionShape = horizontalWritingMode ? createExclusionRectangle(FloatRect(x, y, width, height), FloatSize(radiusX, radiusY)) : createExclusionRectangle(FloatRect(y, x, height, width), FloatSize(radiusY, radiusX)); If the cost of stacking the additional variables is measurable and significant in this case I'd be happy to use the first version. If not, I think the added readability of the current version is worthwhile. > Source/WebCore/rendering/ExclusionShape.cpp:98 > + float centerX = floatValueForLength(circle->centerX(), boxWidth); > + float centerY = floatValueForLength(circle->centerY(), boxHeight);

Hans Muller

Comment 18 2012-09-24 11:06:51 PDT

Created attachment 165413 [details] Patch

Dirk Schulze

Comment 19 2012-09-24 14:03:07 PDT

Comment on attachment 165413 [details] Patch To be honest, I am not very happy that you don't store the FloatRect and FloatSize values in the Exclusion* classes. That would look a lot cleaner IMO. However, at this point it might be over engineering to prepare the code for that.

WebKit Review Bot

Comment 20 2012-09-24 14:18:36 PDT

Comment on attachment 165413 [details] Patch Clearing flags on attachment: 165413 Committed r129411: <http://trac.webkit.org/changeset/129411>

WebKit Review Bot

Comment 21 2012-09-24 14:18:41 PDT

All reviewed patches have been landed. Closing bug.

Bear Travis

Comment 22 2012-09-24 14:47:10 PDT

Reopening to attach new patch.

Bear Travis

Comment 23 2012-09-24 14:47:12 PDT

Created attachment 165450 [details] Attached to incorrect bug Merging with master branch

Bear Travis

Comment 24 2012-09-24 14:50:01 PDT

Closing. Accidentally uploaded patch for bug 96157 here.

Note You need to log in before you can comment on or make changes to this bug.