[webkit-reviews] review granted: [Bug 97533] [CSSRegions]Add support for auto-height regions (without region-breaks) : [Attachment 167535] Patch 3

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Mihnea Ovidenie
<mihnea at adobe.com>'s request for review:
Bug 97533: [CSSRegions]Add support for auto-height regions (without
region-breaks)
https://bugs.webkit.org/show_bug.cgi?id=97533

Attachment 167535: Patch 3
https://bugs.webkit.org/attachment.cgi?id=167535&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
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?