[webkit-reviews] review granted: [Bug 74135] [CSS Regions] Auto width is not working for Regions : [Attachment 162057] 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 74135: [CSS Regions] Auto width is not working for Regions
https://bugs.webkit.org/show_bug.cgi?id=74135

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=162057&action=review


r=me but please do the requested changes.

> Source/WebCore/ChangeLog:34
> +	   (RenderRegion):

The individual entries should describe why you needed those changes.

> Source/WebCore/rendering/RenderBox.cpp:2554
> +	   if (!isRenderRegion() || (isRenderRegion() &&
shouldComputeSizeAsReplaced())) {

Techincally this could be simplified to:

if (!isRenderRegion() || shouldComputeSizeAsReplaced())

but it would be less readable.

> Source/WebCore/rendering/RenderRegion.cpp:503
> +    if (m_flowThread) {

> I have followed a similar pattern used in the new flexible box module in
computing preferred logical widths.

Early returns have always been preferred over nested ifs. The fact that some
other part of the code doesn't follow that is not a justification.

> Source/WebCore/rendering/RenderRegion.cpp:511
> +	   if (styleToUse->logicalMinWidth().isFixed() &&
styleToUse->logicalMinWidth().value() > 0)
> +	       minPreferredLogicalWidth = std::max(minPreferredLogicalWidth,
computeContentBoxLogicalWidth(styleToUse->logicalMinWidth().value()));
> +
> +	   if (styleToUse->logicalMaxWidth().isFixed())
> +	       minPreferredLogicalWidth = std::min(minPreferredLogicalWidth,
computeContentBoxLogicalWidth(styleToUse->logicalMaxWidth().value()));

Please add some FIXMEs here as you only handle the <length> case. min-width /
max-width can be <percentage>, ... and the code won't handle those cases
properly.

> Source/WebCore/rendering/RenderRegion.cpp:519
> +LayoutUnit RenderRegion::maxPreferredLogicalWidth() const

Same comments as for minPreferredLogicalWidth.