Bug 72840 – Implement Zoom Property in CSSSStyleApplyProperty.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Luke Macpherson]

Implement Zoom Property in CSSSStyleApplyProperty.

[Luke Macpherson]

Created attachment 116013 [details]
Patch

[Luke Macpherson]

Could someone please review?

[Andreas Kling]

Comment on attachment 116013 [details]
Patch

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

> Source/WebCore/css/CSSStyleApplyProperty.cpp:911
> +        } else if (primitiveValue->primitiveType() == CSSPrimitiveValue::CSS_PERCENTAGE) {
> +            if (float percent = primitiveValue->getFloatValue()) {
> +                resetEffectiveZoom(selector);
> +                selector->setZoom(percent / 100.0f);
> +            }

The applyProperty() version of this code would always reset the effective zoom, even if getFloatValue() returns 0.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:916
> +        } else if (primitiveValue->primitiveType() == CSSPrimitiveValue::CSS_NUMBER) {
> +            if (float number = primitiveValue->getFloatValue()) {
> +                resetEffectiveZoom(selector);
> +                selector->setZoom(number);
> +            }

Same here.

[Luke Macpherson]

Comment on attachment 116013 [details]
Patch

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

>> Source/WebCore/css/CSSStyleApplyProperty.cpp:911
>> +            }
> 
> The applyProperty() version of this code would always reset the effective zoom, even if getFloatValue() returns 0.

Hi Andreas, thanks for the review.
I understand that's the case in order to exactly match the existing behavior, but is the existing behavior sensible?
To my eyes this code treats any zoom of 0 as invalid and does not apply the property on RenderStyle. In that case, do we want zoom: 0; to have a side-effect of resetting the effectiveZoom?

[Andreas Kling]

(In reply to comment #4)
> (From update of attachment 116013 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116013&action=review
> 
> >> Source/WebCore/css/CSSStyleApplyProperty.cpp:911
> >> +            }
> > 
> > The applyProperty() version of this code would always reset the effective zoom, even if getFloatValue() returns 0.
> 
> Hi Andreas, thanks for the review.
> I understand that's the case in order to exactly match the existing behavior, but is the existing behavior sensible?
> To my eyes this code treats any zoom of 0 as invalid and does not apply the property on RenderStyle. In that case, do we want zoom: 0; to have a side-effect of resetting the effectiveZoom?

Is this behavior change detectable by a layout test? If so, we should add one.

In general I prefer refactorings to be "stand-alone" and free from behavior changes. A change like this could be pushed as a follow-up patch.

[Luke Macpherson]

Created attachment 117066 [details]
Patch

[WebKit Review Bot]

Comment on attachment 117066 [details]
Patch

Clearing flags on attachment: 117066

Committed r101499: <http://trac.webkit.org/changeset/101499>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.