Bug 72350 – Pass Aspect Ratio to RenderStyle

• Component: New Bugs
• Status: RESOLVED FIXED

[Fady Samuel]

Pass Aspect Ratio to RenderStyle

[Fady Samuel]

Created attachment 115092 [details]
Patch

[Fady Samuel]

This code does not yet actually work yet. With all the changes to style code lately, I'm not sure how to get aspect ratio in RenderStyle and accessible from RenderBox (where most of the layout logic will be). This seems to be what I need to do but I'm not sure why it's not working. What am I doing wrong? I'd appreciate any ideas. Thanks.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/css/CSSStyleSelector.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 9 files


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

[Ojan Vafai]

Comment on attachment 115092 [details]
Patch

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

> Source/WebCore/css/CSSStyleSelector.cpp:2595
> +    case CSSPropertyWebkitAspectRatio:
> +    {
> +        HANDLE_INHERIT_AND_INITIAL(aspectRatioNumerator, AspectRatioNumerator);
> +        HANDLE_INHERIT_AND_INITIAL(aspectRatioDenominator, AspectRatioDenominator);
> +        if (!value->isAspectRatioValue())
> +            return;
> +        fprintf(stderr, ">>>setting aspect ratio\n");
> +        CSSAspectRatioValue* aspectRatioValue = static_cast<CSSAspectRatioValue*>(value);
> +        m_style->setAspectRatioNumerator(aspectRatioValue->numeratorValue());
> +        m_style->setAspectRatioDenominator(aspectRatioValue->denominatorValue());
> +        return;
> +    }

Not sure if this is your problem, but I don't think you need any of this code. The code in CSSStyleApplyProperty.cpp should do all this work for you. You just need to add a case to the end of applyProperty to the looong list of cases below "// These properties are implemented in the CSSStyleApplyProperty lookup table."

> Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp:39
> +    , m_aspectRatioNumerator(RenderStyle::initialAspectRatioDenominator())

Should be initialAspectRatioNumerator.

[Fady Samuel]

Looks like this was a failure in testing on my part. I was using the parsing test I wrote for my previous patch. It turns out that the style recalc timer was not going off until the end of all the tests and so the style update never made its way into RenderStyle. In order to verify that style is being set correctly and is accessible in RenderBox, I call document.body.offsetHeight to force a style recalc and relayout after each test. fprintfs will not be included in RenderBox in the final patch obviously, this is just for local testing.

[Fady Samuel]

Created attachment 115616 [details]
Patch

[Fady Samuel]

Created attachment 115621 [details]
Patch

[Ojan Vafai]

Comment on attachment 115621 [details]
Patch

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.

[Fady Samuel]

Comment on attachment 115621 [details]
Patch

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

>> Source/WebCore/rendering/style/RenderStyle.cpp:393
>> +        }
> 
> 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?

That's fair, I'll get rid of this change for this patch.

>> LayoutTests/fast/css/aspect-ratio-parsing-tests.html:19
>> +          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.

I guess I'm getting a little ahead of what's necessary for this patch. This is necessary to force the style recalc so that it then forces a relayout. I did this to verify RenderBox is getting the updated style information while testing this code manually. This is not necessary for this patch or this layout test.

[Fady Samuel]

Created attachment 115646 [details]
Patch

[Fady Samuel]

Comment on attachment 115646 [details]
Patch

Clearing flags on attachment: 115646

Committed r100656: <http://trac.webkit.org/changeset/100656>

[Fady Samuel]

All reviewed patches have been landed.  Closing bug.

[Luke Macpherson]

Sorry for the late feedback, I'm just back from holidays so catching up on my email.

I think it would make sense to introduce an AspectRatio value struct containing the numerator and denominator values, and have a single setter + getter on RenderStyle.

You could probably also squash a few bits by having an invalid AspectRatio when the floats are NaN, which would get you away from three setters + getters on RenderStyle down to one, save a boolean on RenderStyle for hasAspectRatio, drop 15-20 lines of code, and might even allow you to use CSSStyleApplyProperty::ApplyPropertyDefault instead of having to define your own.

I'm not sure if CSSAspectRatioValue belongs in CSSPrimitiveValue or not. I prefer how you have it as a separate class personally, but the existing code does dump most of these simple types in there, especially when the css value can be either an identifier or a primitive value.

Finally a question:
Why do you need a separate numerator and denominator? Couldn't this ratio be effectively stored and used as a fractional floating point value instead? Why can't the property accept a single floating point value in the CSS?

[Fady Samuel]

(In reply to comment #13)
> Sorry for the late feedback, I'm just back from holidays so catching up on my email.
> 
> I think it would make sense to introduce an AspectRatio value struct containing the numerator and denominator values, and have a single setter + getter on RenderStyle.
> 
> You could probably also squash a few bits by having an invalid AspectRatio when the floats are NaN, which would get you away from three setters + getters on RenderStyle down to one, save a boolean on RenderStyle for hasAspectRatio, drop 15-20 lines of code, and might even allow you to use CSSStyleApplyProperty::ApplyPropertyDefault instead of having to define your own.
> 
> I'm not sure if CSSAspectRatioValue belongs in CSSPrimitiveValue or not. I prefer how you have it as a separate class personally, but the existing code does dump most of these simple types in there, especially when the css value can be either an identifier or a primitive value.
> 
> Finally a question:
> Why do you need a separate numerator and denominator? Couldn't this ratio be effectively stored and used as a fractional floating point value instead? Why can't the property accept a single floating point value in the CSS?

I suppose I don't but I decided to keep them up to when we do layout to avoid rounding errors. Occasionally in the original experimental patch (see the master bug), I need 1/aspectRatio() rather than aspectRatio(). In retrospect, this is probably unnecessary.

I'm certainly willing to make changes to this, I just wanted to have all the parsing/plumbing out of the way, so that I can focus on layout, I'm sure I'll probably iterate over layout corner cases at least a dozen times.

[Fady Samuel]

(In reply to comment #13)
> Sorry for the late feedback, I'm just back from holidays so catching up on my email.
> 
> I think it would make sense to introduce an AspectRatio value struct containing the numerator and denominator values, and have a single setter + getter on RenderStyle.
> 
> You could probably also squash a few bits by having an invalid AspectRatio when the floats are NaN, which would get you away from three setters + getters on RenderStyle down to one, save a boolean on RenderStyle for hasAspectRatio, drop 15-20 lines of code, and might even allow you to use CSSStyleApplyProperty::ApplyPropertyDefault instead of having to define your own.
> 
> I'm not sure if CSSAspectRatioValue belongs in CSSPrimitiveValue or not. I prefer how you have it as a separate class personally, but the existing code does dump most of these simple types in there, especially when the css value can be either an identifier or a primitive value.
> 
> Finally a question:
> Why do you need a separate numerator and denominator? Couldn't this ratio be effectively stored and used as a fractional floating point value instead? Why can't the property accept a single floating point value in the CSS?

Luke, I plan to implement your suggestions here: https://bugs.webkit.org/show_bug.cgi?id=73037 before closing the master bug. Thank you!