Bug 80484 – Remove CSSStyleSelector's convertToLength method and use CSSPrimitiveValue's version directly.

• Component: CSS
• Status: RESOLVED FIXED

[Luke Macpherson]

Remove CSSStyleSelector's convertToLength method and use CSSPrimitiveValue's version directly.

[Luke Macpherson]

Created attachment 130537 [details]
Patch

[Mike Lawther]

Comment on attachment 130537 [details]
Patch

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

> Source/WebCore/css/CSSPrimitiveValueMappings.h:3713
> +enum LengthConversion { InvalidConversion = 0, FixedIntegerConversion = 1, FixedFloatConversion = 2, AutoConversion = 4, PercentConversion = 8, FractionConversion = 16};

nit: as these are bitmasks, I'd write them in hex as 0x00, 0x01, 0x02, 0x04, 0x08, 0x10 etc.

[Luke Macpherson]

(In reply to comment #2)
> (From update of attachment 130537 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130537&action=review
> 
> > Source/WebCore/css/CSSPrimitiveValueMappings.h:3713
> > +enum LengthConversion { InvalidConversion = 0, FixedIntegerConversion = 1, FixedFloatConversion = 2, AutoConversion = 4, PercentConversion = 8, FractionConversion = 16};
> 
> nit: as these are bitmasks, I'd write them in hex as 0x00, 0x01, 0x02, 0x04, 0x08, 0x10 etc.

Lots of precedent for doing it this way in the code-base. Eg:

Source/JavaScriptCore/parser/Nodes.h:        enum Type { Constant = 1, Getter = 2, Setter = 4 };
Source/JavaScriptCore/runtime/PropertyDescriptor.h:        enum { WritablePresent = 1, EnumerablePresent = 2, ConfigurablePresent = 4};
Source/WebCore/css/CSSParser.h:    enum FillPositionFlag { InvalidFillPosition = 0, AmbiguousFillPosition = 1, XFillPosition = 2, YFillPosition = 4 };
Source/WebCore/css/CSSPrimitiveValueMappings.h:enum LengthConversion { UnsupportedConversion = 0, FixedConversion = 1, AutoConversion = 2, PercentConversion = 4, FractionConversion = 8};
Source/WebCore/rendering/RenderBlock.h:        enum Type { FloatLeft = 1, FloatRight = 2, FloatLeftRight = 3, FloatPositioned = 4, FloatAll = 7 };

[WebKit Review Bot]

Comment on attachment 130537 [details]
Patch

Attachment 130537 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11837886

New failing tests:
transforms/cssmatrix-2d-interface.xhtml

[Luke Macpherson]

Created attachment 130768 [details]
Patch

[Luke Macpherson]

Created attachment 130949 [details]
Manual Merge

[Julien Chaffraix]

Comment on attachment 130949 [details]
Manual Merge

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

The change looks fine to me. One question though.

> Source/WebCore/css/CSSPrimitiveValueMappings.h:3713
> +enum LengthConversion { InvalidConversion = 0, FixedIntegerConversion = 1, FixedFloatConversion = 2, AutoConversion = 4, PercentConversion = 8, FractionConversion = 16};

I agree with Mike here and would rather see hexadecimal or right-shifted numbers used. Even if there is no real consensus on that, a quick grep showed me that the hexadecimal or right-shifted numbers outweigh the examples you showed (especially if there is a pattern like here). Also preferably with an enum split over different lines for readability.

> Source/WebCore/css/CSSPrimitiveValueMappings.h:3716
> +    if (((supported & FixedIntegerConversion) || (supported & FixedFloatConversion)) && isFontRelativeLength() && (!style || !rootStyle))

Is NULL-checking for rootStyle also needed here?

[Luke Macpherson]

Comment on attachment 130949 [details]
Manual Merge

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

>> Source/WebCore/css/CSSPrimitiveValueMappings.h:3716
>> +    if (((supported & FixedIntegerConversion) || (supported & FixedFloatConversion)) && isFontRelativeLength() && (!style || !rootStyle))
> 
> Is NULL-checking for rootStyle also needed here?

Yes it is, and this patch does. (See the last statement of this line.)

[Luke Macpherson]

Created attachment 131275 [details]
Patch

[Julien Chaffraix]

Comment on attachment 131275 [details]
Patch

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

> Source/WebCore/css/CSSPrimitiveValueMappings.h:3724
> +    if (((supported & FixedIntegerConversion) || (supported & FixedFloatConversion)) && isFontRelativeLength() && (!style || !rootStyle))
> +        return Length(Undefined);

>> Is NULL-checking for rootStyle also needed here?
> Yes it is, and this patch does. (See the last statement of this line.)

I didn't ask the question properly: the previous code was not NULL checking |rootStyle| which is why I was asking if it was needed.

[Luke Macpherson]

(In reply to comment #10)
> (From update of attachment 131275 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131275&action=review
> 
> > Source/WebCore/css/CSSPrimitiveValueMappings.h:3724
> > +    if (((supported & FixedIntegerConversion) || (supported & FixedFloatConversion)) && isFontRelativeLength() && (!style || !rootStyle))
> > +        return Length(Undefined);
> 
> >> Is NULL-checking for rootStyle also needed here?
> > Yes it is, and this patch does. (See the last statement of this line.)
> 
> I didn't ask the question properly: the previous code was not NULL checking |rootStyle| which is why I was asking if it was needed.

Oh, right. The reason is that computeLength<Length> calls computeLengthDouble, which will dereference rootStyle without checking if the units are CSS_REMS, and style when units are CSS_EMS or CSS_EXS. Here I've just grouped these unit types together under "isFontRelativeLength" and ensured that both style and rootStyle are non-null.

[WebKit Review Bot]

Comment on attachment 131275 [details]
Patch

Clearing flags on attachment: 131275

Committed r110484: <http://trac.webkit.org/changeset/110484>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.