Bug 97533 - [CSSRegions]Add support for auto-height regions (without region-breaks)
Summary: [CSSRegions]Add support for auto-height regions (without region-breaks)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mihnea Ovidenie
URL:
Keywords:
Depends on:
Blocks: 66645 91097
  Show dependency treegraph
 
Reported: 2012-09-25 00:30 PDT by Mihnea Ovidenie
Modified: 2012-11-05 01:37 PST (History)
7 users (show)

See Also:


Attachments
Patch (49.84 KB, patch)
2012-09-25 07:01 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch 2 (53.37 KB, patch)
2012-10-03 02:42 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch 3 (53.95 KB, patch)
2012-10-08 06:54 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch for landing (56.11 KB, patch)
2012-10-15 05:32 PDT, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihnea Ovidenie 2012-09-25 00:30:14 PDT
Implement processing model for auto-height regions as described in http://dev.w3.org/csswg/css3-regions/#regions-visual-formatting-steps, but without taking region-breaks into account, which will be addressed in a follow up patch.
Comment 1 Mihnea Ovidenie 2012-09-25 07:01:54 PDT
Created attachment 165605 [details]
Patch
Comment 2 WebKit Review Bot 2012-09-25 07:04:52 PDT
Attachment 165605 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/rendering/RenderView.cpp:149:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Julien Chaffraix 2012-09-27 11:10:21 PDT
Comment on attachment 165605 [details]
Patch

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

> Source/WebCore/rendering/FlowThreadController.cpp:179
> +bool FlowThreadController::resetRegionsOverrideLogicalContentHeight()

Not a huge fan of reset functions returning some bool parameter. I don't understand why FlowThreadController::hasAutoLogicalHeightRegions() is not enough as you check it in RenderView::layout().

> Source/WebCore/rendering/FlowThreadController.cpp:185
> +        if (flowRenderer->resetRegionsOverrideLogicalContentHeight())
> +            hadFlowsWithAutoLogicalHeightRegions = true;

Maybe better written:

hadFlowsWithAutoLogicalHeightRegions |= flowRenderer->resetRegionsOverrideLogicalContentHeight();

(removes the need for the branch).

> Source/WebCore/rendering/RenderFlowThread.cpp:226
> +        if (region->needsOverrideLogicalContentHeightComputation())
> +            // If we have an auto logical height region for which we did not compute a height yet,
> +            // then we cannot compute and update the height of this flow.
> +            return;

Please put curly braces around multi-line if () as per our style guide.

> Source/WebCore/rendering/RenderFlowThread.cpp:812
> +        region->setNeedsLayout(true);

Please use the following instead:

region->setNeedsLayout(true, MarkOnlyThis)

Calling setNeedsLayout(true, MarkContainingBlockChain) during layout is a programming mistake that we should ASSERT against (but that's another debate). Here I think it is not too bad but I would rather avoid anti-patterns.

> Source/WebCore/rendering/RenderFlowThread.cpp:825
> +        region->setNeedsLayout(true);

Ditto.

> Source/WebCore/rendering/RenderView.cpp:185
>      if (needsLayout()) {

I would really push this needsLayout() call above and make it an early return (before we initialize the LayoutState but after the ASSERT).

> Source/WebCore/rendering/RenderView.cpp:199
> +        m_layoutPhase = RenderViewNormalLayout;
> +        setNeedsLayout(false);

This is confusing to reset the state to the normal layout for the second phase and doesn't make much sense.

Reading the code, you would assume the setNeedsLayout(false) will trigger an ASSERT in layoutContent but no due to marking below. AFAICT, you *know* you should have at least one region to lay out.

> Source/WebCore/rendering/RenderView.h:191
> +    enum RenderViewLayoutPhase { RenderViewNormalLayout, UnconstrainedFlowThreadsLayoutInAutoLogicalHeightRegions };

You are assuming that we always do a normal layout and may do a pre-unconstrained flow thread layout. You could just do an unconstrained phase first (regardless of whether there is any auto-height regions) and then if you had auto-height region do a follow-up constrained phase. I would say this would be more readable.
Comment 4 Mihnea Ovidenie 2012-10-03 02:42:51 PDT
Created attachment 166835 [details]
Patch 2
Comment 5 Mihnea Ovidenie 2012-10-03 02:47:43 PDT
(In reply to comment #3)
> (From update of attachment 165605 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165605&action=review
> 
> > Source/WebCore/rendering/FlowThreadController.cpp:179
> > +bool FlowThreadController::resetRegionsOverrideLogicalContentHeight()
> 
> Not a huge fan of reset functions returning some bool parameter. I don't understand why FlowThreadController::hasAutoLogicalHeightRegions() is not enough as you check it in RenderView::layout().
> 

You are right, resetRegionsOverrideLogicalContentHeight has void return type.

> > Source/WebCore/rendering/FlowThreadController.cpp:185
> > +        if (flowRenderer->resetRegionsOverrideLogicalContentHeight())
> > +            hadFlowsWithAutoLogicalHeightRegions = true;
> 
> Maybe better written:
> 
> hadFlowsWithAutoLogicalHeightRegions |= flowRenderer->resetRegionsOverrideLogicalContentHeight();
> 
> (removes the need for the branch).
> 
> > Source/WebCore/rendering/RenderFlowThread.cpp:226
> > +        if (region->needsOverrideLogicalContentHeightComputation())
> > +            // If we have an auto logical height region for which we did not compute a height yet,
> > +            // then we cannot compute and update the height of this flow.
> > +            return;
> 
> Please put curly braces around multi-line if () as per our style guide.
> 

Done.

> > Source/WebCore/rendering/RenderFlowThread.cpp:812
> > +        region->setNeedsLayout(true);
> 
> Please use the following instead:
> 
> region->setNeedsLayout(true, MarkOnlyThis)
> 
> Calling setNeedsLayout(true, MarkContainingBlockChain) during layout is a programming mistake that we should ASSERT against (but that's another debate). Here I think it is not too bad but I would rather avoid anti-patterns.
>

Added a FIXME in the code and call region->setNeedsLayout(true) for now.
 
> > Source/WebCore/rendering/RenderFlowThread.cpp:825
> > +        region->setNeedsLayout(true);
> 
> Ditto.
> 

Added a FIXME in the code and call region->setNeedsLayout(true) for now.

> > Source/WebCore/rendering/RenderView.cpp:185
> >      if (needsLayout()) {
> 
> I would really push this needsLayout() call above and make it an early return (before we initialize the LayoutState but after the ASSERT).
> 

Done.

> > Source/WebCore/rendering/RenderView.cpp:199
> > +        m_layoutPhase = RenderViewNormalLayout;
> > +        setNeedsLayout(false);
> 
> This is confusing to reset the state to the normal layout for the second phase and doesn't make much sense.
> 
> Reading the code, you would assume the setNeedsLayout(false) will trigger an ASSERT in layoutContent but no due to marking below. AFAICT, you *know* you should have at least one region to lay out.
> 

I have removed the call to setNeedsLayout(false).

> > Source/WebCore/rendering/RenderView.h:191
> > +    enum RenderViewLayoutPhase { RenderViewNormalLayout, UnconstrainedFlowThreadsLayoutInAutoLogicalHeightRegions };
> 
> You are assuming that we always do a normal layout and may do a pre-unconstrained flow thread layout. You could just do an unconstrained phase first (regardless of whether there is any auto-height regions) and then if you had auto-height region do a follow-up constrained phase. I would say this would be more readable.

Ok, now the 2 phases of layout are { RenderViewNormalLayout, ConstrainedFlowThreadsLayoutInAutoLogicalHeightRegions }. RenderViewNormalLayout is done regardless we have auto logical height regions, the second one is done only when we have auto logical height regions and we need a second pass.
Comment 6 Julien Chaffraix 2012-10-04 09:53:17 PDT
Comment on attachment 166835 [details]
Patch 2

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

> LayoutTests/fast/regions/autoheight-horizontal-bt.html:26
> +                aaaa aaaa aaaa

Shouldn't there be 3 repetitions of 5 'a' to have 3 lines of 200px each?

> LayoutTests/fast/regions/webkit-named-flow-first-empty-region-index.html:11
> -    #region, #region2, region3 {width: 250px; height: 50px}
> +    #region, #region2, #region3 {width: 250px; height: 50px; }

Was the test passing before?

> Source/WebCore/rendering/FlowThreadController.cpp:183
> +        RenderNamedFlowThread* flowRenderer = *iter;
> +        flowRenderer->resetRegionsOverrideLogicalContentHeight();

No need for the extra variable here: (*iter)->resetRegionsOverrideLogicalContentHeight()

> Source/WebCore/rendering/FlowThreadController.cpp:191
> +        RenderNamedFlowThread* flowRenderer = *iter;
> +        flowRenderer->markAutoLogicalHeightRegionsForLayout();

Ditto.

> Source/WebCore/rendering/RenderFlowThread.cpp:637
> +    bool useHorizontalWritingMode = isHorizontalWritingMode();

Unneeded change, also the variable name is worse than the API name. You *are* in an horizontal writing-mode, you don't *use* it.

> Source/WebCore/rendering/RenderFlowThread.cpp:839
> +        if (region->hasOverrideHeight() && view()->normalLayoutPhase()) {
> +            regionLogicalHeight = region->overrideLogicalContentHeight();
> +            regionRect.setHeight(regionLogicalHeight);
> +        }

Are you ignoring the overridden logical content height for the constrained phase as it is already included into the logicalHeight?

Couldn't you ignore the overridden logical content height for the normal phase and properly update your flow-thread in the follow-up phase?

> Source/WebCore/rendering/RenderFlowThread.cpp:864
> +    bool useHorizontalWritingMode = isHorizontalWritingMode();

Same comment about the variable here.

> Source/WebCore/rendering/RenderFlowThread.cpp:882
> +    // Mark all the remaining auto logical height regions with 0 override computed height
> +    for (; regionIter != m_regionList.end(); ++regionIter) {
> +        RenderRegion* currRegion = *regionIter;
> +        if (!currRegion->isValid() || (currRegion == region))
> +            continue;
> +        currRegion->setOverrideLogicalContentHeight(ZERO_LAYOUT_UNIT);
> +    }

You may as well just clearOverrideSize() to avoid a hash lookup and extra-operations later (or better add a clearOverrideLogicalContentHeight())

> Source/WebCore/rendering/RenderRegion.cpp:593
> +    if (view()->normalLayoutPhase())
> +        return;
> +
> +    if (hasAutoLogicalHeight() && hasOverrideHeight()) {

Wouldn't the second check cover the first one as we only set the override height in computeOverflow, ie at the end of the normal layout phase.

> Source/WebCore/rendering/RenderView.cpp:202
> +        m_layoutPhase = RenderViewNormalLayout;

The logic is really backwards here (not that it doesn't work, just that it opens a hole for potential badness). It would be better to always reset m_layoutPhase before calling the first layoutContent.

> Source/WebCore/rendering/RenderView.h:191
> +    enum RenderViewLayoutPhase { RenderViewNormalLayout, ConstrainedFlowThreadsLayoutInAutoLogicalHeightRegions };

Not super happy with the "normal" layout vs ("abnormal") constrained layout phase, I could live with it though as I couldn't find a better way to put it.
Comment 7 Mihnea Ovidenie 2012-10-08 06:15:10 PDT
(In reply to comment #6)
> (From update of attachment 166835 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166835&action=review
> 
> > LayoutTests/fast/regions/autoheight-horizontal-bt.html:26
> > +                aaaa aaaa aaaa
> 
> Shouldn't there be 3 repetitions of 5 'a' to have 3 lines of 200px each?
> 

No, if i have a line with 5'a', that line would have 250px. I have changed the test to make it more clear.

> > LayoutTests/fast/regions/webkit-named-flow-first-empty-region-index.html:11
> > -    #region, #region2, region3 {width: 250px; height: 50px}
> > +    #region, #region2, #region3 {width: 250px; height: 50px; }
> 
> Was the test passing before?
> 

YES. When debugging the patch, i noticed that this test was having an auto-logical height region, which was obviously not the intent here, so i decided to change the test.

> > Source/WebCore/rendering/FlowThreadController.cpp:183
> > +        RenderNamedFlowThread* flowRenderer = *iter;
> > +        flowRenderer->resetRegionsOverrideLogicalContentHeight();
> 
> No need for the extra variable here: (*iter)->resetRegionsOverrideLogicalContentHeight()
> 

OK.

> > Source/WebCore/rendering/FlowThreadController.cpp:191
> > +        RenderNamedFlowThread* flowRenderer = *iter;
> > +        flowRenderer->markAutoLogicalHeightRegionsForLayout();
> 
> Ditto.
>

OK.
 
> > Source/WebCore/rendering/RenderFlowThread.cpp:637
> > +    bool useHorizontalWritingMode = isHorizontalWritingMode();
> 
> Unneeded change, also the variable name is worse than the API name. You *are* in an horizontal writing-mode, you don't *use* it.
> 

Agree.

> > Source/WebCore/rendering/RenderFlowThread.cpp:839
> > +        if (region->hasOverrideHeight() && view()->normalLayoutPhase()) {
> > +            regionLogicalHeight = region->overrideLogicalContentHeight();
> > +            regionRect.setHeight(regionLogicalHeight);
> > +        }
> 
> Are you ignoring the overridden logical content height for the constrained phase as it is already included into the logicalHeight?
>
Yes. 
> Couldn't you ignore the overridden logical content height for the normal phase and properly update your flow-thread in the follow-up phase?
>

Only for this patch, because i am not processing breaks. If it take a break into account and call addForcedBreak from RenderBlock::applyBeforeBreak/applyAfterBreak (not only from RenderFlowThread::computeOverflowStateForRegions), then i need to update the flow thread region rects so that the following auto logical height regions will have a chance to properly paginate flow thread content.
 
> > Source/WebCore/rendering/RenderFlowThread.cpp:864
> > +    bool useHorizontalWritingMode = isHorizontalWritingMode();
> 
> Same comment about the variable here.
> 
> > Source/WebCore/rendering/RenderFlowThread.cpp:882
> > +    // Mark all the remaining auto logical height regions with 0 override computed height
> > +    for (; regionIter != m_regionList.end(); ++regionIter) {
> > +        RenderRegion* currRegion = *regionIter;
> > +        if (!currRegion->isValid() || (currRegion == region))
> > +            continue;
> > +        currRegion->setOverrideLogicalContentHeight(ZERO_LAYOUT_UNIT);
> > +    }
> 
> You may as well just clearOverrideSize() to avoid a hash lookup and extra-operations later (or better add a clearOverrideLogicalContentHeight())
> 

I will add a new function, clearOverrideLogicalContentHeight for this.

> > Source/WebCore/rendering/RenderRegion.cpp:593
> > +    if (view()->normalLayoutPhase())
> > +        return;
> > +
> > +    if (hasAutoLogicalHeight() && hasOverrideHeight()) {
> 
> Wouldn't the second check cover the first one as we only set the override height in computeOverflow, ie at the end of the normal layout phase.
>
> > Source/WebCore/rendering/RenderView.cpp:202
> > +        m_layoutPhase = RenderViewNormalLayout;
> 
> The logic is really backwards here (not that it doesn't work, just that it opens a hole for potential badness). It would be better to always reset m_layoutPhase before calling the first layoutContent.
> 
OK
> > Source/WebCore/rendering/RenderView.h:191
> > +    enum RenderViewLayoutPhase { RenderViewNormalLayout, ConstrainedFlowThreadsLayoutInAutoLogicalHeightRegions };
> 
> Not super happy with the "normal" layout vs ("abnormal") constrained layout phase, I could live with it though as I couldn't find a better way to put it.
I am not happy either with the name, i thought for a better name but without much luck.

Patch coming.
Comment 8 Mihnea Ovidenie 2012-10-08 06:54:25 PDT
Created attachment 167535 [details]
Patch 3
Comment 9 Julien Chaffraix 2012-10-12 11:32:02 PDT
Comment on attachment 167535 [details]
Patch 3

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

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

Let's remove those useless lines from the ChangeLog.

> Source/WebCore/rendering/RenderFlowThread.cpp:871
> +    bool useHorizontalWritingMode = isHorizontalWritingMode();

If you need to cache the variable, use:

bool isHorizontalWritingMode = this->isHorizontalWritingMode();

Alternatively shouldBeConsideredHorizontalWritingMode or equivalent would work. The word 'use' doesn't really fit in this context IMHO.

> Source/WebCore/rendering/RenderFlowThread.cpp:885
> +    for (; regionIter != m_regionList.end(); ++regionIter) {
> +        RenderRegion* currRegion = *regionIter;
> +        if (!currRegion->isValid() || (currRegion == region) || !currRegion->hasAutoLogicalHeight())

You could remove the need to check (currRegion == region) by tweaking the code a bit AFAICT.

// Skip current region.
++regionIter;
for (; regionItern != m_regionList.end(); ++regionIter) {

> Source/WebCore/rendering/RenderRegion.cpp:238
> -void RenderRegion::repaintFlowThreadContentRectangle(const LayoutRect& repaintRect, bool immediate, const LayoutRect& flowThreadPortionRect, const LayoutRect& flowThreadPortionOverflowRect, const LayoutPoint& regionLocation) const
> +void RenderRegion::repaintFlowThreadContentRectangle(const LayoutRect& repaintRect, bool immediate, const LayoutRect& flowThreadPortionRect,
> +    const LayoutRect& flowThreadPortionOverflowRect, const LayoutPoint& regionLocation) const

Unneeded line break.

> Source/WebCore/rendering/RenderRegion.cpp:616
> +    if (view()->normalLayoutPhase())
> +        return !hasOverrideHeight();
> +
> +    return false;

Could be also written:

return view()->normalLayoutPhase() && !hasOverrideHeight();

> Source/WebCore/rendering/RenderView.cpp:190
> +    bool needsTwoPassLayoutForAutoLogicalHeightRegions = hasRenderNamedFlowThreads()
> +        && flowThreadController()->hasAutoLogicalHeightRegions();

Unneeded line breaking.

> Source/WebCore/rendering/RenderView.cpp:206
> +        layoutContent();
> +#ifndef NDEBUG
> +        checkLayoutState(state);
> +#endif

The checkLayoutState should be moved into layoutContent() as it will guarantee it's always called after you layout.

> Source/WebCore/rendering/RenderView.cpp:-174
> -    ASSERT(layoutDelta() == LayoutSize());
> -    ASSERT(m_layoutStateDisableCount == 0);
> -    ASSERT(m_layoutState == &state);

I really liked having these ASSERTs here as they ensured we always were in a good state before clearing m_layoutState. I would introduce a checkLayoutState here even if it is redundant with the other ones.

> Source/WebCore/rendering/RenderView.h:257
> +    void checkLayoutState(const LayoutState&);

#ifndef NDEBUG?
Comment 10 Mihnea Ovidenie 2012-10-15 05:32:41 PDT
Created attachment 168687 [details]
Patch for landing
Comment 11 WebKit Review Bot 2012-10-15 13:31:50 PDT
Comment on attachment 168687 [details]
Patch for landing

Clearing flags on attachment: 168687

Committed r131348: <http://trac.webkit.org/changeset/131348>
Comment 12 WebKit Review Bot 2012-10-15 13:31:54 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Simon Fraser (smfr) 2012-10-15 14:14:07 PDT
Looks like it breaks in release:

http://build.webkit.org/builders/Apple%20Lion%20Release%20%28Build%29/builds/5086/steps/compile-webkit/logs/stdio
/Volumes/Data/slave/lion-release/build/Source/WebCore/rendering/RenderView.cpp:136:51: error: unused parameter 'state' [-Werror,-Wunused-parameter]
void RenderView::layoutContent(const LayoutState& state)
Comment 14 Mark Lam 2012-10-15 14:41:59 PDT
(In reply to comment #13)
> Looks like it breaks in release:
> 
> http://build.webkit.org/builders/Apple%20Lion%20Release%20%28Build%29/builds/5086/steps/compile-webkit/logs/stdio
> /Volumes/Data/slave/lion-release/build/Source/WebCore/rendering/RenderView.cpp:136:51: error: unused parameter 'state' [-Werror,-Wunused-parameter]
> void RenderView::layoutContent(const LayoutState& state)

Landed fix in r131363: <http://trac.webkit.org/changeset/131363>.