Bug 66143

Created attachment 104659 [details] Patch V1 RenderFlowThread::layout will set the current flow thread on its own RenderView. RenderFlowThread cannot be nested inside a single RenderView, so there's no need to keep it on the LayoutState. The RenderFlowThread uses the width of the maximum region, but when it comes to measuring the width of a specific line, the width is adjusted by the difference between the largest region and the region that contains that line.

Created attachment 104660 [details] Patch V2 Using fixedOffset instead of right in RenderBlock::logicalRightOffsetForLine, so that heightRemaining still works correctly.

Comment on attachment 104660 [details] Patch V2 View in context: https://bugs.webkit.org/attachment.cgi?id=104660&action=review Just need some minor tweaks. > Source/WebCore/rendering/RenderBlock.cpp:1229 > LayoutStateMaintainer statePusher(view(), this, locationOffset(), hasColumns() || hasTransform() || hasReflection() || style()->isFlippedBlocksWritingMode(), pageLogicalHeight, pageLogicalHeightChanged, colInfo); > > + bool disableRegionFitting = view()->hasRenderFlowThread() && (hasColumns() || (isPositioned() && !isRenderFlowThread()) || isFloating()); > + RegionFittingDisabler regionFittingDisabler(view()->currentRenderFlowThread(), disableRegionFitting); Pull view() into a local since it's being accessed three times here in quick succession. > Source/WebCore/rendering/RenderBlock.cpp:3576 > + if (view()->hasRenderFlowThread()) { > + RenderFlowThread* flowThread = view()->currentRenderFlowThread(); > + if (flowThread->isRegionFittingEnabled()) { > + LayoutState* layoutState = view()->layoutState(); > + IntSize delta = layoutState->m_layoutOffset - layoutState->m_pageOffset; > + int offset = isHorizontalWritingMode() ? delta.height() : delta.width(); > + LayoutUnit regionWidth = flowThread->regionLogicalWidthForLine(offset + logicalTop); > + right -= flowThread->logicalWidth() - regionWidth; > + } > + } Pull this into a helper function. Even a little static function would be nice. I'm not sure if there's any compelling logic behind picking the right side to shrink or if you just made an arbitrary call. It's cool that this gets lines working, but we know long-term that this solution isn't going to work for block splitting. I guess it's better than nothing though. > Source/WebCore/rendering/RenderFlowThread.cpp:466 > + // FIXME: The regions are always in order, optimize this search. Interval tree might work here right? > Source/WebCore/rendering/RenderFlowThread.h:108 > + RenderRegion* renderRegionForLine(LayoutUnit logicalTop) const; This isn't a "logicalTop" really. It's just a position that is examined. I'd rename "logicalTop" to "position" since that matches the other "ForLine" methods in RenderBlock. > Source/WebCore/rendering/RenderRegion.cpp:89 > + if (m_flowThread && isValid()) > + m_flowThread->invalidateRegions(); Can you go from valid to invalid? If so, then you would need an invalidate for that also. I'm thinking you probably can't, but I figured I'd double-check. I'm assuming going from valid to invalid can only happen across a reconstruction of the RenderObject.

(In reply to comment #3) > (From update of attachment 104660 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=104660&action=review > Pull view() into a local since it's being accessed three times here in quick succession. Ok. > Pull this into a helper function. Even a little static function would be nice. Ok. > I'm not sure if there's any compelling logic behind picking the right side to shrink or if you just made an arbitrary call. I was thinking that regions stay on the left, and I would need to adjust just the right edge. > It's cool that this gets lines working, but we know long-term that this solution isn't going to work for block splitting. I guess it's better than nothing though. For blocks, I think two passes will be necessary. One with the region width size + region styling applied, and if the block fits inside a single region we're done, if not do a second pass with the maximum region width. I'm not sure how would that work with incremental layout. > > > Source/WebCore/rendering/RenderFlowThread.cpp:466 > > + // FIXME: The regions are always in order, optimize this search. > > Interval tree might work here right? Can that be in a separate bug? I'm sure a simple binary search will do here, because the regions are always in order and never overlapping. Just that we now have a linked list and it would be slow to search the middle region. > This isn't a "logicalTop" really. It's just a position that is examined. I'd rename "logicalTop" to "position" since that matches the other "ForLine" methods in RenderBlock. Ok. > Can you go from valid to invalid? If so, then you would need an invalidate for that also. I'm thinking you probably can't, but I figured I'd double-check. I'm assuming going from valid to invalid can only happen across a reconstruction of the RenderObject. No, there should be no way to get from valid to invalid. The state is required for regions that are invalid when first added. At a later time other regions may be removed meaning that some of the invalid regions may become valid. That's why we need to keep track of invalid regions. Once valid a region cannot go back. A visible issue of this approach might be that one could remove a valid region and reverse the dependencies. When the same region is added back, this time it becomes invalid.

Created attachment 104722 [details] Patch V3

Comment on attachment 104722 [details] Patch V3 r=me

Comment on attachment 104722 [details] Patch V3 Clearing flags on attachment: 104722 Committed r93538: <http://trac.webkit.org/changeset/93538>

All reviewed patches have been landed. Closing bug.

This seems to have caused the Leopard and SnowLeopard bots, as well as the Chromium Mac bot, to fail fast/repaint/japanese-rl-selection-repaint-in-regions.html . Below I've pasted a diff from the Leopard bot. This looks like it could just be a test whose results need updating, but I don't know anything about this area, so I'm not really sure. The Chromium Mac bots are also failing downstream for the Chromium folks. Since we run pixel tests, there are also pixel diffs in addition to textual diffs. Because ATM neither achicu or dhyatt are around on #webkit to ask about this stuff, I'm rolling this change back for now. --- /Volumes/Data/slave/leopard-intel-debug-tests/build/layout-test-results/fast/repaint/japanese-rl-selection-repaint-in-regions-expected.txt +++ /Volumes/Data/slave/leopard-intel-debug-tests/build/layout-test-results/fast/repaint/japanese-rl-selection-repaint-in-regions-actual.txt @@ -9,8 +9,8 @@ Thread with flow-name 'thread' layer at (400,0) size 400x400 RenderFlowThread at (0,0) size 400x400 - RenderBlock {DIV} at (0,0) size 480x400 - RenderText {#text} at (5,0) size 469x388 + RenderBlock {DIV} at (0,0) size 1248x400 + RenderText {#text} at (5,0) size 1237x388 text run at (5,0) width 381: "\x{305B}\x{3063}\x{304B}\x{304F}\x{898B}\x{3064}\x{3051}\x{305F}\x{3059}\x{3070}\x{3089}\x{3057}\x{3044}\x{8A18}\x{4E8B}\x{304C}\x{3069}\x{3053}" text run at (37,0) width 381: "\x{306B}\x{3042}\x{3063}\x{305F}\x{304B}\x{5FD8}\x{308C}\x{3066}\x{3057}\x{307E}\x{3063}\x{305F}\x{7D4C}\x{9A13}\x{306F}\x{3042}\x{308A}\x{307E}" text run at (69,0) width 360: "\x{3059}\x{304B}\x{306A}\x{3089}\x{30BF}\x{30A4}\x{30C8}\x{30EB}\x{3068}\x{30A2}\x{30C9}\x{30EC}\x{30B9}\x{3060}\x{3051}\x{3067}\x{306A}" @@ -24,8 +24,32 @@ text run at (325,0) width 381: "\x{30DA}\x{30FC}\x{30B8}\x{306E}\x{30B3}\x{30F3}\x{30C6}\x{30F3}\x{30C4}\x{304B}\x{3089}\x{3082}\x{691C}\x{7D22}\x{3059}\x{308B}\x{3053}\x{3068}" text run at (357,0) width 381: "\x{304C}\x{3067}\x{304D}\x{307E}\x{3059}\x{3002}\x{305B}\x{3063}\x{304B}\x{304F}\x{898B}\x{3064}\x{3051}\x{305F}\x{3059}\x{3070}\x{3089}\x{3057}" text run at (389,0) width 381: "\x{3044}\x{8A18}\x{4E8B}\x{304C}\x{3069}\x{3053}\x{306B}\x{3042}\x{3063}\x{305F}\x{304B}\x{5FD8}\x{308C}\x{3066}\x{3057}\x{307E}\x{3063}\x{305F}" - text run at (421,0) width 388: "\x{7D4C}\x{9A13}\x{306F}\x{3042}\x{308A}\x{307E}\x{3059}\x{304B} \x{306A}\x{3089}\x{30BF}\x{30A4}\x{30C8}\x{30EB}\x{3068}\x{30A2}\x{30C9}\x{30EC}" - text run at (453,0) width 191: "\x{30B9}\x{3060}\x{3051}\x{3067}\x{306A}\x{304F}\x{3001}\x{8A2A}\x{554F}" + text run at (421,0) width 22: "\x{7D4C}" + text run at (453,0) width 22: "\x{9A13}" + text run at (485,0) width 22: "\x{306F}" + text run at (517,0) width 22: "\x{3042}" + text run at (549,0) width 22: "\x{308A}" + text run at (581,0) width 22: "\x{307E}" + text run at (613,0) width 22: "\x{3059}" + text run at (645,0) width 22: "\x{304B}" + text run at (677,0) width 22: "\x{306A}" + text run at (709,0) width 22: "\x{3089}" + text run at (741,0) width 22: "\x{30BF}" + text run at (773,0) width 22: "\x{30A4}" + text run at (805,0) width 22: "\x{30C8}" + text run at (837,0) width 22: "\x{30EB}" + text run at (869,0) width 22: "\x{3068}" + text run at (901,0) width 22: "\x{30A2}" + text run at (933,0) width 22: "\x{30C9}" + text run at (965,0) width 22: "\x{30EC}" + text run at (997,0) width 22: "\x{30B9}" + text run at (1029,0) width 22: "\x{3060}" + text run at (1061,0) width 22: "\x{3051}" + text run at (1093,0) width 22: "\x{3067}" + text run at (1125,0) width 22: "\x{306A}" + text run at (1157,0) width 43: "\x{304F}\x{3001}" + text run at (1189,0) width 22: "\x{8A2A}" + text run at (1221,0) width 22: "\x{554F}" Regions for flow 'thread' RenderRegion {DIV} with index 0 selection start: position 28 of child 0 {#text} of child 1 {DIV} of body

Reverted r93538 for reason: Broke Leopard, SnowLeopard, and Chromium Mac bots Committed r93556: <http://trac.webkit.org/changeset/93556>

(In reply to comment #10) > Reverted r93538 for reason: > > Broke Leopard, SnowLeopard, and Chromium Mac bots > > Committed r93556: <http://trac.webkit.org/changeset/93556> Sorry about that, that test would need to be updated, but I'm not sure why it didn't show up when I run the tests or the chromium-bot did. For example, it isn't here http://queues.webkit.org/results/9467898 .

Created attachment 104786 [details] Patch V4 Extended the last region width to all the content that doesn't fit into the regions and that will also fix the test.

Comment on attachment 104786 [details] Patch V4 r=me

Comment on attachment 104786 [details] Patch V4 Clearing flags on attachment: 104786 Committed r93627: <http://trac.webkit.org/changeset/93627>

All reviewed patches have been landed. Closing bug.