[webkit-reviews] review denied: [Bug 73277] -webkit-aspect-ratio Layout Implementation : [Attachment 116873] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Fady Samuel <fsamuel at chromium.org>'s
request for review:
Bug 73277: -webkit-aspect-ratio Layout Implementation
https://bugs.webkit.org/show_bug.cgi?id=73277

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
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.