Bug 96267 – [CSSRegions]Flag auto-height regions

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

[Mihnea Ovidenie]

First split of auto-height regions bigger patch.

[Mihnea Ovidenie]

Created attachment 163592 [details]
Patch

[WebKit Review Bot]

Attachment 163592 [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
/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:34: DeprecationWarning: the sets module is deprecated
  import sets
WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky.
LayoutTests/platform/chromium/TestExpectations:2503:  Unsupported expectation: FAIL  [test/expectations] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Julien Chaffraix]

Comment on attachment 163592 [details]
Patch

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

The patch doesn't apply on ToT. I would like to see a revised version but the approach looks OK.

>> LayoutTests/platform/chromium/TestExpectations:2503
>> +BUGWK96267 SKIP : fast/regions/autoheight-regions-mark.html = FAIL
> 
> Unsupported expectation: FAIL  [test/expectations] [5]

I think you just put PASS instead of FAIL as you are SKIP'ping them.

> Source/WebCore/ChangeLog:11
> +        - has height: auto and is part of the normal flow
> +        - has height: auto, is not part of normal flow and does not have top/bottom specified

Let's use precise language here. You are doing the right thing in the code by looking at the *logical* height and *logical* top/bottom but this ChangeLog could lead people to think otherwise.

> Source/WebCore/rendering/RenderRegion.cpp:304
> +    if (isValid()) {

Nit: Early return are preferred.

> Source/WebCore/rendering/RenderRegion.h:118
> +    bool usesAutoHeight() const { return m_usesAutoHeight; }

Not a huge fan of the naming, first it should be usesAutoLogicalHeight. But the whole hasAutoHeightStyle vs usesAutoHeight is not super clear to me.

In RenderLayer, we use a pattern of should* / is* (shouldBeNormalFlowOnly / isNormalFlowOnly, shouldBeSelfPaintingLayer / isSelfPaintingLayer). It would fit nicely here.

> Source/WebCore/rendering/RenderRegion.h:195
> +    bool m_usesAutoHeight;

May be better to move it with the other booleans above.

> Source/WebCore/rendering/RenderView.cpp:160
> +#ifndef NDEBUG
> +        if (m_flowThreadController)
> +            m_flowThreadController->checkAutoHeightRegions();
> +#endif

Pushing that into layoutRenderNamedFlowThreads() would have made more sense but that means losing some coverage as the check wouldn't be run if you have no named flow thread. This could easily be fixed by moving the count check for named flow thread into layoutRenderNamedFlowThreads.

[Mihnea Ovidenie]

Created attachment 164389 [details]
Patch 2

[Mihnea Ovidenie]

(In reply to comment #3)
> (From update of attachment 163592 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163592&action=review
> 
> The patch doesn't apply on ToT. I would like to see a revised version but the approach looks OK.
> 
> >> LayoutTests/platform/chromium/TestExpectations:2503
> >> +BUGWK96267 SKIP : fast/regions/autoheight-regions-mark.html = FAIL
> > 
> > Unsupported expectation: FAIL  [test/expectations] [5]
> 
> I think you just put PASS instead of FAIL as you are SKIP'ping them.
> 

Done.

> > Source/WebCore/ChangeLog:11
> > +        - has height: auto and is part of the normal flow
> > +        - has height: auto, is not part of normal flow and does not have top/bottom specified
> 
> Let's use precise language here. You are doing the right thing in the code by looking at the *logical* height and *logical* top/bottom but this ChangeLog could lead people to think otherwise.
> 

Done.

> > Source/WebCore/rendering/RenderRegion.cpp:304
> > +    if (isValid()) {
> 
> Nit: Early return are preferred.
> 

Done.

> > Source/WebCore/rendering/RenderRegion.h:118
> > +    bool usesAutoHeight() const { return m_usesAutoHeight; }
> 
> Not a huge fan of the naming, first it should be usesAutoLogicalHeight. But the whole hasAutoHeightStyle vs usesAutoHeight is not super clear to me.
> 
> In RenderLayer, we use a pattern of should* / is* (shouldBeNormalFlowOnly / isNormalFlowOnly, shouldBeSelfPaintingLayer / isSelfPaintingLayer). It would fit nicely here.
> 

I used shouldHaveAutoLogicalHeight() and hasAutoLogicalHeight(). I preferred these names instead of shouldBeAutoLogicalHeight/isAutoLogicalHeight.

> > Source/WebCore/rendering/RenderRegion.h:195
> > +    bool m_usesAutoHeight;
> 
> May be better to move it with the other booleans above.
> 

Done, also made the booleans bit sized.

> > Source/WebCore/rendering/RenderView.cpp:160
> > +#ifndef NDEBUG
> > +        if (m_flowThreadController)
> > +            m_flowThreadController->checkAutoHeightRegions();
> > +#endif
> 
> Pushing that into layoutRenderNamedFlowThreads() would have made more sense but that means losing some coverage as the check wouldn't be run if you have no named flow thread. This could easily be fixed by moving the count check for named flow thread into layoutRenderNamedFlowThreads.

Done.

[Julien Chaffraix]

Comment on attachment 164389 [details]
Patch 2

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

> LayoutTests/ChangeLog:14
> +        * platform/mac/fast/regions/autoheight-regions-mark-expected.png: Added.
> +        * platform/mac/fast/regions/autoheight-regions-mark-expected.txt: Added.

Unless I am mistaking, those baselines are cross-platform and could be moved next to the test case.

> Source/WebCore/rendering/FlowThreadController.cpp:104
> +#ifndef NDEBUG
> +    checkAutoLogicalHeightRegions();
> +#endif

An alternative to that would be to change checkAutoLogicalHeightRegions to isAutoLogicalHeightRegionsFlagConsistent and make it return a boolean so that you can just do:

ASSERT(isAutoLogicalHeightRegionsFlagConsistent());

That would be my preferred way.

> Source/WebCore/rendering/RenderRegion.cpp:206
> +    checkRegionHavingAutoLogicalHeight();

Checking is more about a 'check' (for example your check in RenderFlowThread), not really an 'update'. I would name it updateRegionHasAutoLogicalHeightFlag, recomputeRegionAutoLogicalHeightFlag or similar.

> Source/WebCore/rendering/RenderTreeAsText.cpp:698
> +                    if (renderRegion->hasAutoLogicalHeight())
> +                        ts << " hasAutoLogicalHeight";

This really looks like it should be in RenderRegion::renderName(). As this is the existing pattern, I am not asking to change it now but you should consider that.

[Mihnea Ovidenie]

Created attachment 164510 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 164510 [details]
Patch for landing

Clearing flags on attachment: 164510

Committed r128861: <http://trac.webkit.org/changeset/128861>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.