Bug 72840 - Implement Zoom Property in CSSSStyleApplyProperty.
Summary: Implement Zoom Property in CSSSStyleApplyProperty.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Luke Macpherson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-20 17:10 PST by Luke Macpherson
Modified: 2011-11-30 05:48 PST (History)
4 users (show)

See Also:


Attachments
Patch (7.42 KB, patch)
2011-11-20 17:12 PST, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (7.36 KB, patch)
2011-11-29 16:09 PST, Luke Macpherson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Macpherson 2011-11-20 17:10:28 PST
Implement Zoom Property in CSSSStyleApplyProperty.
Comment 1 Luke Macpherson 2011-11-20 17:12:33 PST
Created attachment 116013 [details]
Patch
Comment 2 Luke Macpherson 2011-11-22 18:17:42 PST
Could someone please review?
Comment 3 Andreas Kling 2011-11-27 05:23:29 PST
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.
Comment 4 Luke Macpherson 2011-11-27 14:45:14 PST
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?
Comment 5 Andreas Kling 2011-11-29 14:40:41 PST
(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.
Comment 6 Luke Macpherson 2011-11-29 16:09:55 PST
Created attachment 117066 [details]
Patch
Comment 7 WebKit Review Bot 2011-11-30 05:48:42 PST
Comment on attachment 117066 [details]
Patch

Clearing flags on attachment: 117066

Committed r101499: <http://trac.webkit.org/changeset/101499>
Comment 8 WebKit Review Bot 2011-11-30 05:48:46 PST
All reviewed patches have been landed.  Closing bug.