Bug 96267

Created attachment 163592 [details] Patch

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.

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.

Created attachment 164389 [details] Patch 2

(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.

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.

Created attachment 164510 [details] Patch for landing

Comment on attachment 164510 [details] Patch for landing Clearing flags on attachment: 164510 Committed r128861: <http://trac.webkit.org/changeset/128861>

All reviewed patches have been landed. Closing bug.