Bug 66143 - [CSSRegions] RenderFlowThread layout should use the attached region sizes
Summary: [CSSRegions] RenderFlowThread layout should use the attached region sizes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexandru Chiculita
URL:
Keywords:
Depends on:
Blocks: 57312 66198
  Show dependency treegraph
 
Reported: 2011-08-12 09:41 PDT by Alexandru Chiculita
Modified: 2011-08-23 12:45 PDT (History)
4 users (show)

See Also:


Attachments
Patch V1 (29.78 KB, patch)
2011-08-22 03:40 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff
Patch V2 (29.79 KB, patch)
2011-08-22 04:08 PDT, Alexandru Chiculita
hyatt: review-
hyatt: commit-queue-
Details | Formatted Diff | Diff
Patch V3 (31.68 KB, patch)
2011-08-22 13:16 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff
Patch V4 (38.52 KB, patch)
2011-08-22 21:03 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandru Chiculita 2011-08-12 09:41:44 PDT
Currently the RenderFlowThread is taking the maximum size of the attached regions. It should flow the content inside the specified region sizes.
Comment 1 Alexandru Chiculita 2011-08-22 03:40:04 PDT
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.
Comment 2 Alexandru Chiculita 2011-08-22 04:08:38 PDT
Created attachment 104660 [details]
Patch V2

Using fixedOffset instead of right in RenderBlock::logicalRightOffsetForLine, so that heightRemaining still works correctly.
Comment 3 Dave Hyatt 2011-08-22 12:08:39 PDT
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.
Comment 4 Alexandru Chiculita 2011-08-22 12:29:46 PDT
(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.
Comment 5 Alexandru Chiculita 2011-08-22 13:16:38 PDT
Created attachment 104722 [details]
Patch V3
Comment 6 Dave Hyatt 2011-08-22 13:23:35 PDT
Comment on attachment 104722 [details]
Patch V3

r=me
Comment 7 WebKit Review Bot 2011-08-22 14:04:46 PDT
Comment on attachment 104722 [details]
Patch V3

Clearing flags on attachment: 104722

Committed r93538: <http://trac.webkit.org/changeset/93538>
Comment 8 WebKit Review Bot 2011-08-22 14:04:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Peter Kasting 2011-08-22 15:52:41 PDT
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
Comment 10 Peter Kasting 2011-08-22 16:15:29 PDT
Reverted r93538 for reason:

Broke Leopard, SnowLeopard, and Chromium Mac bots

Committed r93556: <http://trac.webkit.org/changeset/93556>
Comment 11 Alexandru Chiculita 2011-08-22 20:22:18 PDT
(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 .
Comment 12 Alexandru Chiculita 2011-08-22 21:03:08 PDT
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 13 Dave Hyatt 2011-08-23 12:21:38 PDT
Comment on attachment 104786 [details]
Patch V4

r=me
Comment 14 WebKit Review Bot 2011-08-23 12:44:56 PDT
Comment on attachment 104786 [details]
Patch V4

Clearing flags on attachment: 104786

Committed r93627: <http://trac.webkit.org/changeset/93627>
Comment 15 WebKit Review Bot 2011-08-23 12:45:01 PDT
All reviewed patches have been landed.  Closing bug.