[webkit-reviews] review granted: [Bug 35770] CSSPrimitiveValue.getFloatValue does not convert sizes : [Attachment 73760] [PATCH] Comments addressed
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Nov 15 10:06:24 PST 2010
Simon Fraser (smfr) <simon.fraser at apple.com> has granted Alexander Pavlov
(apavlov) <apavlov at chromium.org>'s request for review:
Bug 35770: CSSPrimitiveValue.getFloatValue does not convert sizes
https://bugs.webkit.org/show_bug.cgi?id=35770
Attachment 73760: [PATCH] Comments addressed
https://bugs.webkit.org/attachment.cgi?id=73760&action=review
------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=73760&action=review
r=me with comments addressed.
> WebCore/ChangeLog:11
> + CSSPrimitiveValue.getFloatValue does not convert sizes
> +
> + Implemented all same-category unit conversions (length, angle, time,
frequency) and left
> + absolute-relative (cm, mm, in, pt, pc) length conversions intact
(assuming a monitor resolution of 96ppi).
> + Illegal unit conversion attempts will throw an INVALID_ACCESS_ERR
DOMException.
> + https://bugs.webkit.org/show_bug.cgi?id=35770
> +
The bug URL should go above the title.
> WebCore/css/CSSPrimitiveValue.cpp:54
> + // Here we violate the spec
(http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-CSSPrimitiveValue)
> + // and allow conversions between CSS_PX and relative length units
assuming a 96ppi screen (see CSSHelper.h).
Why? Please justify.
> WebCore/css/CSSPrimitiveValue.cpp:91
> // A more stylish solution than sharing would be to turn CSSPrimitiveValue
(or CSSValues in general) into non-virtual,
> -// non-refcounted simple type with value semantics. In practice these
sharing tricks get similar memory benefits
> +// non-refcounted simple type with value semantics. In practice these
sharing tricks get similar memory benefits
This sentence needs a period at the end.
> WebCore/css/CSSPrimitiveValue.cpp:459
> static double scaleFactorForConversion(unsigned short unitType)
This function has a really bad name. It would be better named as
conversionToCanonicalUnitsScaleFactor(). Also, who decides what the canonical
units are?
> WebCore/css/CSSPrimitiveValue.cpp:505
> double CSSPrimitiveValue::getDoubleValue(unsigned short unitType,
ExceptionCode& ec)
Can all the getFooValue methods be |const|?
More information about the webkit-reviews
mailing list