Use Optionals in RenderBox height computations
Created attachment 257427 [details] Patch
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.
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
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.
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
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.
Created attachment 259590 [details] Patch
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.
Created attachment 259592 [details] Patch
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
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
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
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
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).
(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?
Created attachment 259695 [details] Patch
Created attachment 259696 [details] Patch
Created attachment 259697 [details] Patch
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.
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 :(
Created attachment 259752 [details] Patch
(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.
Comment on attachment 259752 [details] Patch r=me
Committed r188873: <http://trac.webkit.org/changeset/188873>
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.