Bug 147253 – Use Optionals in RenderBox height computations

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Use Optionals in RenderBox height computations

[Myles C. Maxfield]

Created attachment 257427 [details]
Patch

[Build Bot]

Comment on attachment 257427 [details]
Patch

Attachment 257427 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5825036246056960

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 257430 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 257427 [details]
Patch

Attachment 257427 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5275606849683456

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 257434 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Darin Adler]

Comment on attachment 257427 [details]
Patch

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

Tests are failing so there must be a logic mistake here.

> Source/WebCore/rendering/RenderBox.cpp:689
> -    return std::max(logicalHeight, computeLogicalHeightUsing(styleToUse.logicalMinHeight(), intrinsicContentHeight));
> +    return std::max(logicalHeight, computeLogicalHeightUsing(styleToUse.logicalMinHeight(), intrinsicContentHeight).valueOr(-1));

Why write it this way, with the magic -1? How about a local variable and an if statement instead? Or you could make a WTF::max function template that allows one of the two arguments to be optional and ignores the one that is not present.

> Source/WebCore/rendering/RenderBox.cpp:699
> -    return std::max(logicalHeight, computeContentLogicalHeight(styleToUse.logicalMinHeight(), intrinsicContentHeight));
> +    return std::max(logicalHeight, computeContentLogicalHeight(styleToUse.logicalMinHeight(), intrinsicContentHeight).valueOr(-1));

Ditto.

> Source/WebCore/rendering/RenderBox.cpp:2824
> +    return Optional<LayoutUnit>(Nullopt);

Why the cast? I’d expect you could just return Nullopt.

> Source/WebCore/rendering/RenderBox.cpp:2831
> +    return Optional<LayoutUnit>(Nullopt);

Why the cast? I’d expect you could just return Nullopt.

> Source/WebCore/rendering/RenderBox.cpp:2856
> +    return Optional<LayoutUnit>(Nullopt);

Why the cast? I’d expect you could just return Nullopt.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:671
> -    return std::max(LayoutUnit::fromPixel(0), computeMainAxisExtentForChild(child, MainOrPreferredSize, flexBasis));
> +    return std::max(LayoutUnit::fromPixel(0), computeMainAxisExtentForChild(child, MainOrPreferredSize, flexBasis).valueOr(0));

Why write it this way, with the magic 0? How about a local variable and an if statement instead? Or you could make a WTF::max function template that allows one of the two arguments to be optional and ignores the one that is not present.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:857
>      LayoutUnit minExtent = 0;
>      if (min.isSpecifiedOrIntrinsic())
> -        minExtent = computeMainAxisExtentForChild(child, MinSize, min);
> +        minExtent = computeMainAxisExtentForChild(child, MinSize, min).valueOr(childSize);
>      return std::max(childSize, minExtent);

I think it’s non-obvious to use valueOr(childSize) for minExtent. I think valueOr(0) would make more sense.

> Source/WebCore/rendering/RenderMultiColumnSet.cpp:419
> +        if (Optional<LayoutUnit> logicalMaxHeight = multicolBlock->computeContentLogicalHeight(multicolStyle.logicalMaxHeight(), -1))
> +            maxColumnHeight = std::max(maxColumnHeight, logicalMaxHeight.value());

Could write this without an if statement if we had the new WTF::max I suggested above.

[Myles C. Maxfield]

Created attachment 259590 [details]
Patch

[WebKit Commit Bot]

Attachment 259590 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 10 files


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

[Myles C. Maxfield]

Created attachment 259592 [details]
Patch

[Build Bot]

Comment on attachment 259592 [details]
Patch

Attachment 259592 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/85049

New failing tests:
fast/css-grid-layout/flex-and-minmax-content-resolution-rows.html
fast/css-grid-layout/grid-item-removal-auto-placement-update.html
fast/css-grid-layout/minmax-min-content-column-resolution-rows.html
fast/css-grid-layout/percent-track-breadths-regarding-container-size.html
fast/css-grid-layout/grid-item-addition-auto-placement-update.html
fast/css-grid-layout/grid-item-with-percent-height-in-auto-height-grid-resolution.html
fast/css-grid-layout/implicit-rows-auto-resolution.html
fast/css-grid-layout/implicit-position-dynamic-change.html
fast/css-grid-layout/float-not-protruding-into-next-grid-item.html
fast/css-grid-layout/grid-auto-flow-resolution.html
fast/css-grid-layout/grid-item-negative-indexes.html
css3/flexbox/percentage-sizes.html
fast/css-grid-layout/grid-auto-columns-rows-resolution.html
fast/css-grid-layout/grid-element-min-max-width.html
fast/css-grid-layout/grid-auto-columns-rows-auto-flow-resolution.html
fast/css-grid-layout/grid-item-negative-integer-explicit-grid-resolution.html
fast/css-grid-layout/grid-item-removal-track-breadth-update.html
fast/css-grid-layout/grid-auto-columns-rows-update.html
fast/css-grid-layout/minmax-max-content-resolution-rows.html
fast/css-grid-layout/grid-item-addition-track-breadth-update.html
fast/css-grid-layout/auto-content-resolution-rows.html
fast/css-grid-layout/grid-item-multiple-minmax-content-resolution.html
fast/css-grid-layout/minmax-spanning-resolution-rows.html
fast/css-grid-layout/grid-dynamic-updates-relayout.html
fast/css-grid-layout/percent-intrinsic-track-breadth.html

[Build Bot]

Created attachment 259595 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 259592 [details]
Patch

Attachment 259592 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/85086

New failing tests:
fast/css-grid-layout/flex-and-minmax-content-resolution-rows.html
fast/css-grid-layout/grid-element-min-max-width.html
fast/css-grid-layout/minmax-min-content-column-resolution-rows.html
fast/css-grid-layout/percent-track-breadths-regarding-container-size.html
fast/css-grid-layout/grid-item-addition-auto-placement-update.html
fast/css-grid-layout/grid-item-with-percent-height-in-auto-height-grid-resolution.html
fast/css-grid-layout/implicit-rows-auto-resolution.html
fast/css-grid-layout/implicit-position-dynamic-change.html
fast/css-grid-layout/float-not-protruding-into-next-grid-item.html
fast/css-grid-layout/grid-auto-flow-resolution.html
fast/css-grid-layout/grid-item-negative-indexes.html
css3/flexbox/percentage-sizes.html
fast/css-grid-layout/grid-auto-columns-rows-resolution.html
fast/css-grid-layout/grid-item-removal-auto-placement-update.html
fast/css-grid-layout/grid-auto-columns-rows-auto-flow-resolution.html
fast/css-grid-layout/grid-item-negative-integer-explicit-grid-resolution.html
fast/css-grid-layout/grid-item-removal-track-breadth-update.html
fast/css-grid-layout/grid-auto-columns-rows-update.html
fast/css-grid-layout/minmax-max-content-resolution-rows.html
fast/css-grid-layout/grid-item-addition-track-breadth-update.html
fast/css-grid-layout/auto-content-resolution-rows.html
fast/css-grid-layout/grid-item-multiple-minmax-content-resolution.html
fast/css-grid-layout/minmax-spanning-resolution-rows.html
fast/css-grid-layout/grid-dynamic-updates-relayout.html
fast/css-grid-layout/percent-intrinsic-track-breadth.html

[Build Bot]

Created attachment 259596 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Anders Carlsson]

Comment on attachment 259592 [details]
Patch

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

> Source/WTF/wtf/MathExtras.h:414
> +template<typename T>
> +T max(T a, Optional<T> b)
> +{
> +    return b ? std::max(a, b.value()) : a;
> +}
> +
> +template<typename T>
> +T min(T a, Optional<T> b)
> +{
> +    return b ? std::min(a, b.value()) : a;
> +}

I don't think we should add these. Ordering between Optionals is undefined (and the reason std::optional wasn't included in the C++14 standard).

[Myles C. Maxfield]

(In reply to comment #14)
> Comment on attachment 259592 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=259592&action=review
> 
> > Source/WTF/wtf/MathExtras.h:414
> > +template<typename T>
> > +T max(T a, Optional<T> b)
> > +{
> > +    return b ? std::max(a, b.value()) : a;
> > +}
> > +
> > +template<typename T>
> > +T min(T a, Optional<T> b)
> > +{
> > +    return b ? std::min(a, b.value()) : a;
> > +}
> 
> I don't think we should add these. Ordering between Optionals is undefined
> (and the reason std::optional wasn't included in the C++14 standard).

I don't understand what you mean by "ordering." Could you explain?

[Myles C. Maxfield]

Created attachment 259695 [details]
Patch

[Myles C. Maxfield]

Created attachment 259696 [details]
Patch

[Myles C. Maxfield]

Created attachment 259697 [details]
Patch

[Myles C. Maxfield]

Specifically looking for reviews from Dave Hyatt and Javier Fernandez. In particular, this patch adds some pesky .valueOr(-1)s to grid layout code and I'm hoping Javier can tell me how to get rid of them.

[Myles C. Maxfield]

Comment on attachment 257427 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.cpp:689
>> +    return std::max(logicalHeight, computeLogicalHeightUsing(styleToUse.logicalMinHeight(), intrinsicContentHeight).valueOr(-1));
> 
> Why write it this way, with the magic -1? How about a local variable and an if statement instead? Or you could make a WTF::max function template that allows one of the two arguments to be optional and ignores the one that is not present.

Anders told me not to make a WTF::max() function.

>> Source/WebCore/rendering/RenderBox.cpp:2824
>> +    return Optional<LayoutUnit>(Nullopt);
> 
> Why the cast? I’d expect you could just return Nullopt.

I thought so too, but it didn't work :(

[Myles C. Maxfield]

Created attachment 259752 [details]
Patch

[Myles C. Maxfield]

(In reply to comment #20)
> Comment on attachment 257427 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257427&action=review
> 
> >> Source/WebCore/rendering/RenderBox.cpp:689
> >> +    return std::max(logicalHeight, computeLogicalHeightUsing(styleToUse.logicalMinHeight(), intrinsicContentHeight).valueOr(-1));
> > 
> > Why write it this way, with the magic -1? How about a local variable and an if statement instead? Or you could make a WTF::max function template that allows one of the two arguments to be optional and ignores the one that is not present.
> 
> Anders told me not to make a WTF::max() function.
> 
> >> Source/WebCore/rendering/RenderBox.cpp:2824
> >> +    return Optional<LayoutUnit>(Nullopt);
> > 
> > Why the cast? I’d expect you could just return Nullopt.
> 
> I thought so too, but it didn't work :(

Looks like it does work. I wonder what I was doing wrong before.

[Dave Hyatt]

Comment on attachment 259752 [details]
Patch

r=me

[Myles C. Maxfield]

Committed r188873: <http://trac.webkit.org/changeset/188873>

[Javier Fernandez]

Comment on attachment 259752 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:1883
> +        return overrideContainingBlockContentLogicalWidth().valueOr(-1);

I'm not really sure whether we could get rid of this -1. I tend to think that fir this logic and thinking only about grid layout implications, having a -1 would be similar to not having an override value, so result should be get from availbleLogicalWidth.

> Source/WebCore/rendering/RenderBox.cpp:1895
> +        return overrideContainingBlockContentLogicalHeight().valueOr(-1);

Ditto.

> Source/WebCore/rendering/RenderBox.cpp:2942
> +    if (!availableHeight)

Since we used before -1, we expected to return here in case of containingBlockChild->containingBlockLogicalWidthForContent() being -1 when called some lines before. Hence, I don't think the current logic is equivalent to the previous one. I'm not really sure how to deal with this, besides let containingBlockLogicalWidthForContent and containingBlockLogicalHeightForContent returning an Optional value as well. However, I think that's not easy because it relies on availableLogicalHeight/Width result when the grid flag is not enabled.

> Source/WebCore/rendering/RenderGrid.cpp:1313
> +        bool canShrinkToFitInRowAxisForChild = !hasAutoMinSizeInRowAxis || (child.overrideContainingBlockContentLogicalWidth() && child.minPreferredLogicalWidth() <= child.overrideContainingBlockContentLogicalWidth().value());

Grid uses overrideContainingBlockContentLogicalWidth/Height to override grid item's containing block width/height with the abstract "grid item's area" breadth. This is not an actual block, but something logical we define for grid layout to place the grid items. The grid areas breadth are defined and set in the RenderGrid::layoutGridItems function. The function we are changing here is the stretching logic, which needs the grid areas to be defined and set appropriately before it can be executed (if alignment indicates so and required conditions are fulfilled).

We can safely assume both  overrideContainingBlockContentLogicalWidth/Heigh have values different than -1/NullOpt at this point. Actually, perhaps we should ASSERT that.

> Source/WebCore/rendering/RenderGrid.cpp:1317
> +            LayoutUnit childWidthToFitContent = std::max(std::min(child.maxPreferredLogicalWidth(), child.overrideContainingBlockContentLogicalWidth().valueOr(-1) - child.marginLogicalWidth()), child.minPreferredLogicalWidth());

As commented before, we can assume a value was set. The -1 "magic" value was a way to indicate the override value was undefined, which is not possible for executing this logic So here, we could use just value().

> Source/WebCore/rendering/RenderGrid.cpp:1331
> +            LayoutUnit stretchedLogicalHeight = availableAlignmentSpaceForChildBeforeStretching(child.overrideContainingBlockContentLogicalHeight().valueOr(-1), child);

Ditto.