Bug 99216 – [CSS Exclusions] Add ExclusionShape::shapeBoundingBox() method

• Component: CSS
• Status: RESOLVED FIXED

[Hans Muller]

We need a method that returns an ExclusionShape's physical bounds.  We already have shapeLogicalBoundingBox().

[Hans Muller]

Created attachment 168509 [details]
Patch

[WebKit Review Bot]

Attachment 168509 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/rendering/ExclusionShape.cp..." exit_code: 1
Source/WebCore/rendering/ExclusionShape.cpp:140:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hans Muller]

Created attachment 168510 [details]
Patch

Fixed check-webkit-style problems.

[Build Bot]

Comment on attachment 168510 [details]
Patch

Attachment 168510 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14293083

New failing tests:
fast/js/random-array-gc-stress.html

[Hans Muller]

Created attachment 168738 [details]
Patch

Added FloatRect::extend() to simplify writing loops that accumulate the bounding box for a sequence of vertices.  For example:

FloatRect box(vertices[0], FloatSize(0,0));
for (unsigned i = 1; i < vertices.length; i++)
    box.extend(vertices[i]);

ExclusionShape and ExclusionPolygon now use FloatRect::extend().

[Dirk Schulze]

Comment on attachment 168738 [details]
Patch

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

Patch looks great. Just one snippet. And please add the change log and say that it is just clean up. No change of functionality. It already should be covered by existing tests.

> Source/WebCore/platform/graphics/FloatRect.cpp:137
> +void FloatRect::extend(const FloatPoint& p)

Looks great!

> Source/WebCore/rendering/ExclusionShape.cpp:97
>          exclusionShape = horizontalWritingMode
>              ? createExclusionRectangle(FloatRect(x, y, width, height), FloatSize(radiusX, radiusY))
>              : createExclusionRectangle(FloatRect(y, x, height, width), FloatSize(radiusY, radiusX));
> +        exclusionShape->m_boundingBox = FloatRect(x, y, width, height);

It doesn't bring you any performance or memory win, but I would suggest creating the FloatRect before creating the ExclusionRectangle. Seems to be cleaner.

FloatRect rect(floatValueForLength(rectangle->x(), boxWidth),
    floatValueForLength(rectangle->y(), boxHeight),
    floatValueForLength(rectangle->width(), boxWidth),
    floatValueForLength(rectangle->height(), boxHeight));

[Hans Muller]

(In reply to comment #6)
> (From update of attachment 168738 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168738&action=review
...
> It doesn't bring you any performance or memory win, but I would suggest creating the FloatRect before creating the ExclusionRectangle. Seems to be cleaner.
> 
> FloatRect rect(floatValueForLength(rectangle->x(), boxWidth),
>     floatValueForLength(rectangle->y(), boxHeight),
>     floatValueForLength(rectangle->width(), boxWidth),
>     floatValueForLength(rectangle->height(), boxHeight));

Since the createExclusionRectangle() parameter depends on horizontalWritingMode, I assume what you're suggesting is this:

        FloatRect rectangle(
            floatValueForLength(rectangle->x(), boxWidth),
            floatValueForLength(rectangle->y(), boxHeight),
            floatValueForLength(rectangle->width(), boxWidth),
            floatValueForLength(rectangle->height(), boxHeight));
        ...
        FloatSize cornerRadii(radiusX, radiusY);

        exclusionShape = horizontalWritingMode
            ? createExclusionRectangle(rectangle, cornerRadii)
            : createExclusionRectangle(rectangle.transposeRect(), cornerRadii.transposeSize());;

I included the radii as well, since having both createExclusionRectangle() parameters transposed seems clearer (too).  Unfortunately, if I make this change, then for consistency I'd need to change all of the createExclusionShape() cases. I'm not convinced that doing so would make the code that much clearer and I think it really starts to go beyond the scope of this bug.

[Hans Muller]

Created attachment 169273 [details]
Patch

Added a ChangeLog entry.

[Hans Muller]

Created attachment 169321 [details]
Patch

Made the requested changes for ExclusionRectangle in createExclusionShape().

[Dirk Schulze]

Comment on attachment 169321 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 169321 [details]
Patch

Clearing flags on attachment: 169321

Committed r131768: <http://trac.webkit.org/changeset/131768>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.