[webkit-reviews] review denied: [Bug 72350] Pass Aspect Ratio to RenderStyle : [Attachment 115621] 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 72350: Pass Aspect Ratio to RenderStyle
https://bugs.webkit.org/show_bug.cgi?id=72350

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=115621&action=review


Code looks fine. I'm fine with having all this untestable plumbing code, I'm a
bit more hesitant about the RenderStyle.cpp change since it actually has some
logic.

> Source/WebCore/rendering/style/RenderStyle.cpp:393
> +	   if (rareNonInheritedData->m_hasAspectRatio !=
other->rareNonInheritedData->m_hasAspectRatio)
> +	       return StyleDifferenceLayout;
> +
> +	   if (rareNonInheritedData->m_hasAspectRatio) {
> +	       float aspectRatio = rareNonInheritedData->m_aspectRatioNumerator
/ rareNonInheritedData->m_aspectRatioDenominator;
> +	       float otherAspectRatio =
other->rareNonInheritedData->m_aspectRatioNumerator /
other->rareNonInheritedData->m_aspectRatioDenominator;
> +	       if (aspectRatio != otherAspectRatio)
> +		   return StyleDifferenceLayout;
> +	   }

I'm hesitant to add complicated code like this without any tests. I understand
this code can't be tested until you actually are using this values. Maybe this
bit belongs in the patch where you actually modify RenderBox?

> LayoutTests/fast/css/aspect-ratio-parsing-tests.html:19
> +	     // Force a relayout to test
> +	     document.body.offsetHeight;

Is this still necessary? If it is, then I think there's still a bug. You
shouldn't need a layout for the style parsing to happen. You'll need the layout
in order to measure  the actual size of the element, but not to get the inline
style attribute.