Add support for shape-insides that have multiple segments per line (eg, an "H" shaped polygon)
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
Comment on attachment 170038 [details] Initial Patch Attachment 170038 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14505235
Comment on attachment 170038 [details] Initial Patch Attachment 170038 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14481779
Comment on attachment 170038 [details] Initial Patch Attachment 170038 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14489733
Comment on attachment 170038 [details] Initial Patch Attachment 170038 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14484731
Comment on attachment 170038 [details] Initial Patch Attachment 170038 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14492695
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
Comment on attachment 170038 [details] Initial Patch Attachment 170038 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14489744
Created attachment 170278 [details] Updating patch Fixing build issues and problems with the other shape-inside tests.
Comment on attachment 170278 [details] Updating patch Attachment 170278 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14564242
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
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?
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.
Comment on attachment 170722 [details] Incorporating feedback Attachment 170722 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14564707
Comment on attachment 170722 [details] Incorporating feedback Attachment 170722 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14562704
Comment on attachment 170722 [details] Incorporating feedback Attachment 170722 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14555751
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
Created attachment 170757 [details] Fixing inline function issues
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
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
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
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.
Created attachment 173725 [details] Merging with trunk
Comment on attachment 173725 [details] Merging with trunk r- ifdef BidiRun please. Everything else looks good.
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.
Created attachment 176815 [details] Fixing compile guard bug
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.
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.
Comment on attachment 177374 [details] Updating patch r=me
Comment on attachment 177374 [details] Updating patch Clearing flags on attachment: 177374 Committed r136729: <http://trac.webkit.org/changeset/136729>
All reviewed patches have been landed. Closing bug.