Bug 100049 – [CSS Exclusions] Implement empty segments for multiple-segment shape-insides

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Bear Travis]

We at least need several tests to cover these cases

[Bear Travis]

Created attachment 198452 [details]
Initial Patch

[Hans Muller]

Source/WebCore/ChangeLog
> Shape-insides do not require content in every segment. 
I'm not exactly sure what this means. Some line segments within a shape may be empty because because the interior of the shape does not completely overlap the line?


Source/WebCore/rendering/RenderBlockLineLayout.cpp

>       float minWidth = 0;
>       for (size_t i = 0; i < wordMeasurements.size(); i++) {
>           minWidth = wordMeasurements[i].width;
>           if (minWidth > 0)
>               break;
>       }

Maybe move this litte loop into an aptly named inline function, so:

    float minWidth = firstPositiveWidth(wordMeasurements);

And I'm hoping that there's no such thing as negative WordMeasurement.width values.  Note also: the cosmetic feedback I've been getting about this kind of loop is to use unsigned and ++i, as in: for (unsigned i = 0; i < wordMeasurements.size(); ++i)

[Bear Travis]

Created attachment 198757 [details]
Incorporating feedback

[Benjamin Poulain]

Comment on attachment 198757 [details]
Incorporating feedback

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

Some minor comments bellow.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1629
> +    for (unsigned i = 0; i < wordMeasurements.size(); i++) {

unsigned-> size_t
i++ -> ++i

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1715
> +            ASSERT(minWidth || !wordMeasurements.size());

!wordMeasurements.size() -> wordMeasurements.isEmpty()

[Bear Travis]

Created attachment 198764 [details]
Updating patch

[Dave Hyatt]

Comment on attachment 198764 [details]
Updating patch

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

r=me, although see comment.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1722
> +        if (exclusionShapeInsideInfo && end == resolver.position()) {
> +            float minWidth = firstPositiveWidth(wordMeasurements);
> +            ASSERT(minWidth || wordMeasurements.isEmpty());
> +            LayoutUnit nextLineHeight;
> +            if (minWidth > 0 && exclusionShapeInsideInfo->adjustLogicalLineTop(minWidth))
> +                nextLineHeight = exclusionShapeInsideInfo->logicalLineTop() - absoluteLogicalTop;
> +            else
> +                nextLineHeight = exclusionShapeInsideInfo->shapeLogicalBottom() - absoluteLogicalTop;
> +            end = restartLayoutRunsAndFloatsInRange(logicalHeight(), nextLineHeight, lastFloatFromPreviousLine, resolver, oldEnd);
>              continue;

I think it would be nice to put this into a helper function, even if it's just a static function that is up with firstPositiveWidth's definition.

[Bear Travis]

Created attachment 198915 [details]
Factoring out line adjustment code

[Bear Travis]

Comment on attachment 198915 [details]
Factoring out line adjustment code

Adding to commit queue.

[WebKit Commit Bot]

Comment on attachment 198915 [details]
Factoring out line adjustment code

Clearing flags on attachment: 198915

Committed r148781: <http://trac.webkit.org/changeset/148781>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.