Bug 72350

Summary: Pass Aspect Ratio to RenderStyle
Product: WebKit Reporter: Fady Samuel <fsamuel>
Component: New BugsAssignee: Fady Samuel <fsamuel>
Status: RESOLVED FIXED    
Severity: Normal CC: macpherson, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 47738    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Fady Samuel 2011-11-14 21:04:35 PST
Pass Aspect Ratio to RenderStyle
Comment 1 Fady Samuel 2011-11-14 21:05:04 PST
Created attachment 115092 [details]
Patch
Comment 2 Fady Samuel 2011-11-14 21:07:45 PST
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.
Comment 3 WebKit Review Bot 2011-11-14 21:07:45 PST
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.
Comment 4 Ojan Vafai 2011-11-16 11:33:44 PST
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.
Comment 5 Fady Samuel 2011-11-16 21:51:27 PST
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.
Comment 6 Fady Samuel 2011-11-17 10:23:22 PST
Created attachment 115616 [details]
Patch
Comment 7 Fady Samuel 2011-11-17 10:50:31 PST
Created attachment 115621 [details]
Patch
Comment 8 Ojan Vafai 2011-11-17 11:40:25 PST
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.
Comment 9 Fady Samuel 2011-11-17 11:48:07 PST
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.
Comment 10 Fady Samuel 2011-11-17 11:58:58 PST
Created attachment 115646 [details]
Patch
Comment 11 Fady Samuel 2011-11-17 12:09:42 PST
Comment on attachment 115646 [details]
Patch

Clearing flags on attachment: 115646

Committed r100656: <http://trac.webkit.org/changeset/100656>
Comment 12 Fady Samuel 2011-11-17 12:09:48 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Luke Macpherson 2011-11-17 15:47:34 PST
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?
Comment 14 Fady Samuel 2011-11-17 15:58:11 PST
(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.
Comment 15 Fady Samuel 2011-11-23 12:19:49 PST
(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!