Remove CSSStyleSelector's convertToLength method and use CSSPrimitiveValue's version directly.
Created attachment 130537 [details] Patch
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.
(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 };
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
Created attachment 130768 [details] Patch
Created attachment 130949 [details] Manual Merge
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?
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.)
Created attachment 131275 [details] Patch
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.
(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.
Comment on attachment 131275 [details] Patch Clearing flags on attachment: 131275 Committed r110484: <http://trac.webkit.org/changeset/110484>
All reviewed patches have been landed. Closing bug.