Bug 80484 - Remove CSSStyleSelector's convertToLength method and use CSSPrimitiveValue's version directly.
: Remove CSSStyleSelector's convertToLength method and use CSSPrimitiveValue's ...
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-03-06 21:23 PST by
Modified: 2012-03-12 14:54 PST (History)


Attachments
Patch (5.88 KB, patch)
2012-03-06 21:26 PST, Luke Macpherson
no flags Review Patch | Details | Formatted Diff | Diff
Patch (6.33 KB, patch)
2012-03-07 22:06 PST, Luke Macpherson
no flags Review Patch | Details | Formatted Diff | Diff
Manual Merge (6.32 KB, patch)
2012-03-08 18:49 PST, Luke Macpherson
no flags Review Patch | Details | Formatted Diff | Diff
Patch (6.36 KB, patch)
2012-03-11 20:27 PST, Luke Macpherson
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-03-06 21:23:04 PST
Remove CSSStyleSelector's convertToLength method and use CSSPrimitiveValue's version directly.
------- Comment #1 From 2012-03-06 21:26:37 PST -------
Created an attachment (id=130537) [details]
Patch
------- Comment #2 From 2012-03-06 21:39:48 PST -------
(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.
------- Comment #3 From 2012-03-06 22:18:13 PST -------
(In reply to comment #2)
> (From update of attachment 130537 [details] [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 };
------- Comment #4 From 2012-03-07 01:29:51 PST -------
(From update of attachment 130537 [details])
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
------- Comment #5 From 2012-03-07 22:06:47 PST -------
Created an attachment (id=130768) [details]
Patch
------- Comment #6 From 2012-03-08 18:49:28 PST -------
Created an attachment (id=130949) [details]
Patch
------- Comment #7 From 2012-03-09 14:44:32 PST -------
(From update of attachment 130949 [details])
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?
------- Comment #8 From 2012-03-11 15:35:33 PST -------
(From update of attachment 130949 [details])
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.)
------- Comment #9 From 2012-03-11 20:27:11 PST -------
Created an attachment (id=131275) [details]
Patch
------- Comment #10 From 2012-03-12 10:13:28 PST -------
(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.
------- Comment #11 From 2012-03-12 14:27:48 PST -------
(In reply to comment #10)
> (From update of attachment 131275 [details] [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.
------- Comment #12 From 2012-03-12 14:54:52 PST -------
(From update of attachment 131275 [details])
Clearing flags on attachment: 131275

Committed r110484: <http://trac.webkit.org/changeset/110484>
------- Comment #13 From 2012-03-12 14:54:57 PST -------
All reviewed patches have been landed.  Closing bug.