Bug 91878 – [CSS Exclusions] Enable shape-inside for multiple-segment polygons

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

[Bear Travis]

Add support for shape-insides that have multiple segments per line (eg, an "H" shaped polygon)

[Bear Travis]

Created attachment 170038 [details]
Initial Patch

Uploading a patch for critique on the general approach. It still needs additional testing, and will not work completely until bug 100039 is fixed

[Early Warning System Bot]

Comment on attachment 170038 [details]
Initial Patch

Attachment 170038 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14505235

[Early Warning System Bot]

Comment on attachment 170038 [details]
Initial Patch

Attachment 170038 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14481779

[WebKit Review Bot]

Comment on attachment 170038 [details]
Initial Patch

Attachment 170038 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14489733

[Build Bot]

Comment on attachment 170038 [details]
Initial Patch

Attachment 170038 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14484731

[Build Bot]

Comment on attachment 170038 [details]
Initial Patch

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

[Peter Beverloo (cr-android ews)]

Comment on attachment 170038 [details]
Initial Patch

Attachment 170038 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14505249

[EFL EWS Bot]

Comment on attachment 170038 [details]
Initial Patch

Attachment 170038 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14489744

[Bear Travis]

Created attachment 170278 [details]
Updating patch

Fixing build issues and problems with the other shape-inside tests.

[Build Bot]

Comment on attachment 170278 [details]
Updating patch

Attachment 170278 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14564242

[WebKit Review Bot]

Comment on attachment 170278 [details]
Updating patch

Attachment 170278 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14569294

New failing tests:
fast/exclusions/shape-inside/shape-inside-multiple-segments-001.html

[Alexandru Chiculita]

Comment on attachment 170278 [details]
Updating patch

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

Looks good! In general I think you can add more comments in the code to explain the magic around multi-line segment splitting. I understand the code now, but I'm not sure we will remember all the decisions we made in a couple of months from now.

I think we will need a bit more code/testing around. Maybe in different patches.
1. The hyphenation on multi-segments. 
2. Modes of alignments: left, right, center, justify

> Source/WebCore/rendering/ExclusionShapeInsideInfo.h:91
> +    SegmentRangeList& segmentRanges()
> +    {
> +        return m_segmentRanges;
> +    }

nit: You can keep this on a single line.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:91
>          // FIXME: Bug 91878: Add support for multiple segments, currently we only support one

I think you can remove the fixme.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:93
> +            m_segment = &exclusionShapeInsideInfo->segments()[exclusionShapeInsideInfo->segmentRanges().size()];

I would move "exclusionShapeInsideInfo->segments()[exclusionShapeInsideInfo->segmentRanges().size()]" to a method inside the ExclusionShapeInsideInfo.

currentSegment() const { return hasSegments() ? m_segments[m_segmentsRanges.size()] : 0; }

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:481
> +        bool canUseExistingParentBox = parentBox && !parentIsConstructedOrHaveNext(parentBox) && (!startNewSegment || parentBox->isRootInlineBox());

Put the value of (!startNewSegment || parentBox->isRootInlineBox()) into a variable of its own and explain why it's needed.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:579
> +        if (!parentBox || parentBox->renderer() != r->m_object->parent() || r->m_startsSegment)

Add a comment explaining why the first bidirun in a segment needs to create parent boxes all the way up to the root line.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:816
> +        if (r->m_startsSegment)

ditto: add a comment explaining what happens with the rest of the bidiruns.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:922
> +            segmentStart = newSegmentStart->next();

Add a comment explaining that you jump over the "barrier" bidi-run.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:926
> +        if (lineBox->knownToHaveNoOverflow() && (minLogicalLeft < startLogicalLeft || maxLogicalRight > endLogicalRight))
> +            lineBox->clearKnownToHaveNoOverflow();

I don't know why this is needed here. It wasn't here in the original function and it is only done for exclusions.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:951
> +        if (r->m_startsSegment)

Add a comment explaining that the caller will call again for the rest of the bidi-runs.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1231
> +            BidiRun* midPoint = createRun(segmentStart.m_pos, segmentStart.m_pos, segmentStart.m_obj, topResolver);

I think we should use a different name for this. MidPoint makes me think at the midpoints used for spacing, but this one is just a marker or barrier that we will use later to separate the bidi-runs.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1234
> +            topResolver.midpointState().betweenMidpoints = false;

Add a comment about the need to reset the midpoint state. I assume that the following segment will have it's own set of midpoints and that's why we need to reset it.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2421
> +            if (lineInfo.previousLineBrokeCleanly()) {

I think you need a test for this check.

> LayoutTests/fast/exclusions/resources/multi-segment-polygon.js:36
> +function simulateWithPolygon(width, height, points) {

Is anyone calling this method?

[Bear Travis]

Created attachment 170722 [details]
Incorporating feedback

Updating patch to include Alex's feedback. Only exception is that the knownToHaveNoOverflow remains and has been factored into endPlacingBoxRangesInInlineDirection.

[Early Warning System Bot]

Comment on attachment 170722 [details]
Incorporating feedback

Attachment 170722 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14564707

[Early Warning System Bot]

Comment on attachment 170722 [details]
Incorporating feedback

Attachment 170722 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14562704

[WebKit Review Bot]

Comment on attachment 170722 [details]
Incorporating feedback

Attachment 170722 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14555751

[Peter Beverloo (cr-android ews)]

Comment on attachment 170722 [details]
Incorporating feedback

Attachment 170722 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14563728

[Bear Travis]

Created attachment 170757 [details]
Fixing inline function issues

[Build Bot]

Comment on attachment 170757 [details]
Fixing inline function issues

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

New failing tests:
fast/exclusions/shape-inside/shape-inside-multiple-segments-003.html
fast/exclusions/shape-inside/shape-inside-multiple-segments-002.html
fast/exclusions/shape-inside/shape-inside-multiple-segments-001.html

[WebKit Review Bot]

Comment on attachment 170757 [details]
Fixing inline function issues

Attachment 170757 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14559941

New failing tests:
fast/exclusions/shape-inside/shape-inside-multiple-segments-003.html
fast/exclusions/shape-inside/shape-inside-multiple-segments-002.html
fast/exclusions/shape-inside/shape-inside-multiple-segments-001.html

[Bear Travis]

Created attachment 171699 [details]
Updating patch

Updating now that bug 100039 has landed. Small tweak in shape-inside-multiple-segments-002 to account for bug 100874

[Bear Travis]

Created attachment 172132 [details]
Including span tests

Based on feedback from dhyatt, including span tests (with some varying line heights). Also linking to bug 101086 to track text alignment issues.

[Bear Travis]

Created attachment 173725 [details]
Merging with trunk

[Dave Hyatt]

Comment on attachment 173725 [details]
Merging with trunk

r-

ifdef BidiRun please. Everything else looks good.

[Bear Travis]

Created attachment 176807 [details]
Conditionally compiling BidiRun::m_startsSegment & minimizing its size

m_startsSegment should only be added when exclusions are enabled. Also moving m_startsSegment to be a bitfield on BidiCharacterRun to avoid adding an entire field.

[Bear Travis]

Created attachment 176815 [details]
Fixing compile guard bug

[Julien Chaffraix]

Comment on attachment 176815 [details]
Fixing compile guard bug

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

> Source/WebCore/ChangeLog:32
> +        (WebCore):

Those lines should be removed as they don't add anything.

> Source/WebCore/ChangeLog:54
> +        * rendering/RenderBlock.cpp:
> +        (WebCore::RenderBlock::exclusionShapeInsideInfoInternal): Factored out calls
> +        to ExclusionShapeInsideInfo to the .cpp file, as ExclusionShapeInsideInfo
> +        requires InlineIterator.h, which would create a circular dependency in
> +        ExclusionShapeInsideInfo.h.
> +        (WebCore):
> +        * rendering/RenderBlock.h:
> +        (WebCore):
> +        (WebCore::RenderBlock::exclusionShapeInsideInfo): Modified to call
> +        exclusionShapeInsideInfoInternal.

I don't think that's the right way to go. You should just de-inline exclusionShapeInsideInfo as there is only one caller to exclusionShapeInsideInfoInternal.

> Source/WebCore/platform/text/BidiResolver.h:153
> +    bool m_startsSegment:1; // See above.

Nit: There is normally a space before and after the ':' though I can't recall this being in our style guide.

Also I would disambiguate the 'see above' as it's not clear which one it refers to:

// Same comment as m_hasHyphen.

> Source/WebCore/rendering/ExclusionShapeInsideInfo.h:82
> +    SegmentRangeList& segmentRanges() { return m_segmentRanges; }

We should probably have 2 versions of this function a const and non-const.

> Source/WebCore/rendering/InlineFlowBox.h:177
> +    void beginPlacingBoxRangesInInlineDirection(float logicalLeft) { setLogicalLeft(logicalLeft); }

This function makes the code less readable as:
* it's basically calling setLogicalLeft (a one liner)
* its name is just not saying anything about what it does.

Let's just remove it.

> Source/WebCore/rendering/InlineFlowBox.h:178
> +    void endPlacingBoxRangesInInlineDirection(float logicalLeft, float logicalRight, float minLogicalLeft, float maxLogicalRight)

Let's rename it to something more descriptive like updateLogicalWidthAndOverflow (feel free to adapt).

> Source/WebCore/rendering/RenderBlock.h:787
> +    InlineFlowBox* createLineBoxes(RenderObject*, const LineInfo&, InlineBox* childBox, bool startsNewSegmnet);

typo: startsNewSegmnet.

We prefer code to use enums instead of booleans as they make the caller a lot more readable. Here it's not that clear cut but I would still prefer an enum.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:475
> +        bool mustCreateBoxesToRoot = startNewSegment && !(parentBox && parentBox->isRootInlineBox());

I tried to understand what this variable means but couldn't really.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:580
> +        bool runStartsSegment =
> +#if ENABLE(CSS_EXCLUSIONS)
> +            r->m_startsSegment;
> +#else
> +            false;
> +#endif

Probably more readable if you put the runStartSegment definition into the 2 #if branches.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:818
> +        // The calculation is separate for each segment

I would put that inside the #if and explain more, technically you should give some 'why' explanation (but I couldn't find any apart that it felt natural):

// We need to justify each segment in an exclusion separately.

This doesn't seem covered by the tests.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:901
> +    bool needsWordSpacing;

What's the need to define needsWordSpacing here as you never use the value outside of the exclusion branch?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:954
> +        // Do not pass the marker for the next segment

Why? <- a 'why' comment would be a lot better here.

[Bear Travis]

Created attachment 177374 [details]
Updating patch

Incorporating feedback from Julien. Making most of the changes suggested above. Still considering how best to address the following:
begin/endPlacingBoxRanges methods are in place because they should ideally be called before/after box ranges are placed. I would still like to emphasize this, even if renaming the methods.
I would like to avoid adding an enumeration for m_startsSegment, as its use is infrequent, and callers either used a passed-in value, or BidiRun::m_startsSegment itself.
needsWordSpacing is used at the end of computeInlineDirectionPositionsForLine.

[Dave Hyatt]

Comment on attachment 177374 [details]
Updating patch

r=me

[WebKit Review Bot]

Comment on attachment 177374 [details]
Updating patch

Clearing flags on attachment: 177374

Committed r136729: <http://trac.webkit.org/changeset/136729>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.