Bug 73277

Created attachment 116873 [details] Patch

Comment on attachment 116873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116873&action=review Just some initial comments that focus mostly on style/readability. I didn't comment too much on the guts of the code since I'm not exactly sure what the right way to do it is. We'll need Hyatt's expertise there. > Source/WebCore/ChangeLog:17 > + More tests and lots of tweaking is coming. This is being uploaded for early review. This sort of comment belongs in the bug, not in the ChangeLog since you would never actually commit this comment. Also, typically, we only mark things for review that are ready to be committed. The way to get an early review is to upload the patch without the r? and ping reviewers asking them to take an early look at it. > Source/WebCore/ChangeLog:29 > + Aspect rati computation happens in three stages. typo: rati > Source/WebCore/rendering/RenderBlock.cpp:1208 > + LayoutUnit previousHeight = logicalHeight(); > + setLogicalHeight(0); Did you need to move this code? > Source/WebCore/rendering/RenderBlock.cpp:1327 > + > + I'm fine with adding a line-break for readability here, but we don't typically use two line-breaks. > Source/WebCore/rendering/RenderBox.cpp:1675 > +bool RenderBox::isWidthDetermined() const > +{ > + return isWidthDeterminedUsing(style()->logicalWidth(), style()->logicalLeft(), style()->logicalRight()); > +} This method look unused to me. > Source/WebCore/rendering/RenderBox.cpp:1680 > + bool logicalMinHeightIsSpecified = !style()->logicalMinHeight().isAuto() && style()->logicalMinHeight().isPositive(); Is there a reason you pass in all the sizes execpt logicalMinHeight? If you passed this in as well, you could change the variables names appropriately and have a single helper function for both isWidthDeterimind and isHeightDetermined. > Source/WebCore/rendering/RenderBox.cpp:1691 > +bool RenderBox::isHeightDetermined() const > +{ > + return isHeightDeterminedUsing(style()->logicalHeight(), style()->logicalTop(), style()->logicalBottom()); > +} This method look unused to me. > Source/WebCore/rendering/RenderBox.cpp:1696 > +bool RenderBox::hasMinOrMaxConstraints() const > +{ > + return !style()->maxHeight().isUndefined() || !style()->minHeight().isZero(); > +} Unused? > Source/WebCore/rendering/RenderBox.cpp:1706 > +bool RenderBox::useAspectRatioForWidth(const Length& logicalWidth, const Length& logicalLeft, const Length& logicalRight) const > +{ > + return !isWidthDeterminedUsing(logicalWidth, logicalLeft, logicalRight) && style()->hasAspectRatio(); > +} > + > +bool RenderBox::useAspectRatioForHeight(const Length& logicalHeight, const Length& logicalTop, const Length& logicalBottom) const > +{ > + return !isHeightDeterminedUsing(logicalHeight, logicalTop, logicalBottom) && style()->hasAspectRatio(); > +} You can turn this into a single function as well once you merge the helper function. In fact, at that point, you don't really need the helper function, you can just inline the code into this function. > Source/WebCore/rendering/RenderBox.cpp:1790 > + if ((logicalWidthLength.isAuto() && !style()->hasAspectRatio()) || hasPerpendicularContainingBlock) { Shouldn't this be checking userAspectRatioForWidth? > Source/WebCore/rendering/RenderBox.cpp:1804 > + const LayoutUnit bordersPlusPadding = borderAndPaddingLogicalWidth(); Can you just inline this call into line 1818? I don't think having this extra local variable increases readability. > Source/WebCore/rendering/RenderBox.cpp:1820 > + switch (widthType) { > + case LogicalWidth: > logicalWidth = style()->logicalWidth(); > - else if (widthType == MinLogicalWidth) > + break; > + case MinLogicalWidth: > logicalWidth = style()->logicalMinWidth(); > - else > + break; > + case MaxLogicalWidth: > logicalWidth = style()->logicalMaxWidth(); > + break; > + case MinPreferredLogicalWidth: > + ASSERT_NOT_REACHED(); > + logicalWidth = Length(minPreferredLogicalWidth() - bordersPlusPadding, Fixed); > + break; > + } This doesn't change any logic, right? Typically, cleanup/refactor patches should be done in a separate patch. That way it's easier to focus on reviewing the logic that actually changed. Also, when a regression is found months later, it's easier if the patches are smaller. In this specific case, I'm not sure this is an improvement. I would prefer the code the way it was with just an ASSERT(widthType != MinPreferredLogicalWidth). > Source/WebCore/rendering/RenderBox.cpp:1826 > + bool useAspectRatio = style()->hasAspectRatio(); I would inline this into the if-statement. WebKit generally errs on the side of conciseness with code style. > Source/WebCore/rendering/RenderBox.cpp:1827 > + if (useAspectRatio) { Should this be calling userAspectRatioForWidth? > Source/WebCore/rendering/RenderBox.cpp:1830 > + if (logicalHeight()) { > + int widthForAspectRatio = static_cast<int>(logicalHeight() * requiredRatio); I don't think you can rely on this having the right value. It will have the value of the last time you did a layout. I think probably all this code doesn't belong here, but I'm sure Hyatt has strong opinions on where it does actually belong. > Source/WebCore/rendering/RenderBox.cpp:2160 > + } else if (style()->hasAspectRatio() && !logicalHeight() && heightType == LogicalHeight) { > + if (logicalWidth()) { > + float requiredRatio = style()->aspectRatioDenominator() / style()->aspectRatioNumerator(); > + int heightForAspectRatio = static_cast<int>(logicalWidth() * requiredRatio); > + logicalHeightValue = heightForAspectRatio; If this chunk of code lived in computeLogicalHeight, you wouldn't have to modify computeLogicalHeightUsing at all. You'd end up modifying a lot less code and IMO, the end result would be cleaner. I'm not a huge fan of this function taking the heightType as an argument instead of the actual height when the only case it needs it for is LogicalHeight. > Source/WebCore/rendering/RenderBox.cpp:2710 > +void RenderBox::computePositionedLogicalWidthUsing(LogicalWidthType widthType, const RenderBoxModelObject* containerBlock, TextDirection containerDirection, I'm not a huge fan of passing in the widthType instead of the length. But I see why you need it. I expect Hyatt's comments will clarify what the best way forward is. > Source/WebCore/rendering/RenderBox.cpp:2887 > + if (useAspectRatio) { This could be "useAspectRatio && widthType == LogicalHeight". That would make the code inside a bit more readable. > Source/WebCore/rendering/RenderBox.cpp:2890 > + if (logicalHeight()) { Same as above, I'm not sure you can use logicalHeight in this function. > Source/WebCore/rendering/RenderBox.cpp:2929 > + float requiredRatio = style()->aspectRatio(); > + if (logicalHeight()) { > + int widthForAspectRatio = static_cast<int>(logicalHeight() * requiredRatio); > + if (widthType == LogicalWidth) > + logicalWidthValue = widthForAspectRatio; > + } else { > + // Height is not determined so LogicalWidth is the container's content width. > + // widthType will be LogicalMinWidth or LogicalMaxWidth only if they are specified in which > + // case they have to be respected. > + if (widthType == LogicalWidth) { > + LayoutUnit availableContainerWidth = containerLogicalWidth - (logicalLeftValue + bordersPlusPadding); > + logicalWidthValue = availableContainerWidth; > + } > + > + } Can you make this a helper function? The code is almost identical with teh block above it. > LayoutTests/css3/aspect-ratio/absolute-ratio-absolute-left-right.html:7 > +<html> > + <head> > + </head> > + <body> > + <div style="position: absolute; top: 10px; left: 10px; right: 10px; -webkit-aspect-ratio: 16 / 9; background-color: green;"></div> > + </body> > +</html> In general, we prefer all new tests to be in standards-mode, so this should start with the html DOCTYPE. Also, you don't need the html, head or body elements. They'll get auto generated. So I would just make this test be: <!DOCTYPE html> <div style="position: absolute; top: 10px; left: 10px; right: 10px; -webkit-aspect-ratio: 16 / 9; background-color: green;"></div> Finally, this is a perfect candidate for being a reftest. Just create absolute-ratio-absolute-left-right-expected.html with an absolutely positioned div that's rendered the same size as this one, but without using aspect-ratio. Obviously, ditto for all the tests below.

Comment on attachment 116873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116873&action=review Replying to some of your comments for my own reference. I will wait for hyatt's response before refreshing this patch! >> Source/WebCore/rendering/RenderBlock.cpp:1208 >> + setLogicalHeight(0); > > Did you need to move this code? Yes, I need to reset the height to 0 before computing width because I make a decision in width computation based on whether height has been set yet or not. Maybe there's a better way to do this though. >> Source/WebCore/rendering/RenderBox.cpp:1675 >> +} > > This method look unused to me. Looks like you're right, I'll remove. >> Source/WebCore/rendering/RenderBox.cpp:1691 >> +} > > This method look unused to me. Looks like you're right (I iterated over this code a lot locally). I will remove it. >> Source/WebCore/rendering/RenderBox.cpp:1696 >> +} > > Unused? Yup, removing. >> Source/WebCore/rendering/RenderBox.cpp:1790 > > Shouldn't this be checking userAspectRatioForWidth? I believe you're right, will fix. >> Source/WebCore/rendering/RenderBox.cpp:1804 >> + const LayoutUnit bordersPlusPadding = borderAndPaddingLogicalWidth(); > > Can you just inline this call into line 1818? I don't think having this extra local variable increases readability. Will do. >> Source/WebCore/rendering/RenderBox.cpp:1820 >> + } > > This doesn't change any logic, right? Typically, cleanup/refactor patches should be done in a separate patch. That way it's easier to focus on reviewing the logic that actually changed. Also, when a regression is found months later, it's easier if the patches are smaller. > > In this specific case, I'm not sure this is an improvement. I would prefer the code the way it was with just an ASSERT(widthType != MinPreferredLogicalWidth). So I'd like to put the aspect ratio code in computeLogical* and computePositionedLogical*, but I'd only like to use it for width/heightType == Logical*. So I did a bit of refactoring, for these methods. >> Source/WebCore/rendering/RenderBox.cpp:1830 >> + int widthForAspectRatio = static_cast<int>(logicalHeight() * requiredRatio); > > I don't think you can rely on this having the right value. It will have the value of the last time you did a layout. I think probably all this code doesn't belong here, but I'm sure Hyatt has strong opinions on where it does actually belong. This is why I moved setLogicalHeight(0). I reset height initially! If logicalHeight() != 0 then we've already called computeLogicalHeight! >> Source/WebCore/rendering/RenderBox.cpp:2160 >> + logicalHeightValue = heightForAspectRatio; > > If this chunk of code lived in computeLogicalHeight, you wouldn't have to modify computeLogicalHeightUsing at all. You'd end up modifying a lot less code and IMO, the end result would be cleaner. I'm not a huge fan of this function taking the heightType as an argument instead of the actual height when the only case it needs it for is LogicalHeight. You're right, I could probably move it out. >> Source/WebCore/rendering/RenderBox.cpp:2887 >> + if (useAspectRatio) { > > This could be "useAspectRatio && widthType == LogicalHeight". That would make the code inside a bit more readable. I will make this change.

Created attachment 117021 [details] Patch

I've only removed unused code at the moment. I haven't made the rest of the changes Ojan suggested just yet.

Comment on attachment 117021 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117021&action=review Some high level comments. > Source/WebCore/rendering/RenderBlock.cpp:1348 > + relayoutChildren = true; This whole code should be under the if (style()->hasAspectRatio()). > Source/WebCore/rendering/RenderBox.cpp:1802 > + ASSERT_NOT_REACHED(); Do we really care about this ASSERT. Even though it documents that the code cannot be reached currently, it does not seem to serve any purpose other than that. Is it really bad to get to this branch? It also creates an artificial discrepancy with the other computeLogicalWidthUsing. If it can be removed then you may consider factoring this code to be shared with the other function (same comment for heightType). > Source/WebCore/rendering/RenderBox.cpp:1815 > + int widthForAspectRatio = static_cast<int>(logicalHeight() * requiredRatio); Should be LayoutUnit as logicalWidthResult is. Use one of the rounding method from LayoutUnit instead of a static_cast. > Source/WebCore/rendering/RenderBox.cpp:2144 > + int heightForAspectRatio = static_cast<int>(logicalWidth() * requiredRatio); LayoutUnit. > Source/WebCore/rendering/RenderBox.cpp:2748 > + int widthForAspectRatio = static_cast<int>(logicalHeight() * requiredRatio); LayoutUnit. > Source/WebCore/rendering/RenderBox.cpp:2876 > + int widthForAspectRatio = static_cast<int>(logicalHeight() * requiredRatio); LayoutUnit. > Source/WebCore/rendering/RenderBox.cpp:2902 > + int widthForAspectRatio = static_cast<int>(logicalHeight() * requiredRatio); LayoutUnit. > Source/WebCore/rendering/RenderBox.cpp:3147 > + int heightForAspectRatio = static_cast<int>(logicalWidth() * requiredRatio); LayoutUnit. > Source/WebCore/rendering/RenderBox.cpp:3226 > + int heightForAspectRatio = static_cast<int>(logicalWidth() * requiredRatio); LayoutUnit. > Source/WebCore/rendering/RenderBox.cpp:3247 > + int heightForAspectRatio = static_cast<int>(logicalWidth() * requiredRatio); LayoutUnit. > LayoutTests/ChangeLog:28 > + * platform/chromium-linux/css3/aspect-ratio/aspect-ratio-stretch-to-fill-expected.txt: Added. Please add some comments on the failing results here.

Created attachment 117037 [details] Patch

Comment on attachment 117021 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117021&action=review Thanks for the input Julien! >> Source/WebCore/rendering/RenderBlock.cpp:1348 >> + relayoutChildren = true; > > This whole code should be under the if (style()->hasAspectRatio()). Good catch! Thanks! >> Source/WebCore/rendering/RenderBox.cpp:1802 >> + ASSERT_NOT_REACHED(); > > Do we really care about this ASSERT. Even though it documents that the code cannot be reached currently, it does not seem to serve any purpose other than that. Is it really bad to get to this branch? It also creates an artificial discrepancy with the other computeLogicalWidthUsing. If it can be removed then you may consider factoring this code to be shared with the other function (same comment for heightType). It can be removed but it shouldn't be reached currently. Removing it. >> Source/WebCore/rendering/RenderBox.cpp:1815 >> + int widthForAspectRatio = static_cast<int>(logicalHeight() * requiredRatio); > > Should be LayoutUnit as logicalWidthResult is. Use one of the rounding method from LayoutUnit instead of a static_cast. Done. >> Source/WebCore/rendering/RenderBox.cpp:2144 >> + int heightForAspectRatio = static_cast<int>(logicalWidth() * requiredRatio); > > LayoutUnit. Done. >> Source/WebCore/rendering/RenderBox.cpp:2748 >> + int widthForAspectRatio = static_cast<int>(logicalHeight() * requiredRatio); > > LayoutUnit. Done. >> Source/WebCore/rendering/RenderBox.cpp:2876 >> + int widthForAspectRatio = static_cast<int>(logicalHeight() * requiredRatio); > > LayoutUnit. Done. >> Source/WebCore/rendering/RenderBox.cpp:2902 >> + int widthForAspectRatio = static_cast<int>(logicalHeight() * requiredRatio); > > LayoutUnit. Done. >> Source/WebCore/rendering/RenderBox.cpp:3147 >> + int heightForAspectRatio = static_cast<int>(logicalWidth() * requiredRatio); > > LayoutUnit. Done. >> Source/WebCore/rendering/RenderBox.cpp:3226 >> + int heightForAspectRatio = static_cast<int>(logicalWidth() * requiredRatio); > > LayoutUnit. Done >> Source/WebCore/rendering/RenderBox.cpp:3247 >> + int heightForAspectRatio = static_cast<int>(logicalWidth() * requiredRatio); > > LayoutUnit. Done. >> LayoutTests/ChangeLog:28 >> + * platform/chromium-linux/css3/aspect-ratio/aspect-ratio-stretch-to-fill-expected.txt: Added. > > Please add some comments on the failing results here. Failing? They're not failing they're expected. These layout tests are temporary. I will refresh them as I iterate over the code here.

Created attachment 117111 [details] Patch

A little bit more clean-up and tweaking coming soon to a theater near you! More tests coming and further refactoring. Will do that tomorrow, most likely.

Comment on attachment 117111 [details] Patch Attachment 117111 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10709453 New failing tests: css3/aspect-ratio/aspect-ratio-auto-height.html css3/aspect-ratio/aspect-ratio-inherit.html css3/aspect-ratio/absolute-ratio-absolute-left-right.html css3/aspect-ratio/aspect-ratio-absolute-width.html css3/aspect-ratio/aspect-ratio-absolute-height.html

The prefixed aspect-ratio was removed, closing.