SVGLengthValue should use two enums for 'type' and 'mode' instead of one unsigned for 'units'

[Said Abou-Hallawa]

If SVGLengthType and SVGLengthMode are converted to enum class type with size unit_8, SVGLengthValue will not need to use the bit operations to store them in on unsigned.

[Said Abou-Hallawa]

Created attachment 378508 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 378508 [details]
Patch

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

> Source/WebCore/svg/SVGLengthValue.cpp:328
> +    if (from.lengthType() == lengthType() || from.isZero() || isZero() || from.lengthType() == SVGLengthType::EMS || from.lengthType() == SVGLengthType::EXS) {

This is pretty confusing.

> Source/WebCore/svg/SVGLengthValue.h:87
> +    void newValueSpecifiedUnits(SVGLengthType, float valueInSpecifiedUnits);

What does "new value" mean here?

[Nikolas Zimmermann]

Comment on attachment 378508 [details]
Patch

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

Nice cleanup, much better to read!

> Source/WebCore/ChangeLog:9
> +        It used to allocate the least significant bits 4 bits to the SVGLengthMode

bits 4 bits -> 4 bits

>> Source/WebCore/svg/SVGLengthValue.h:87
>> +    void newValueSpecifiedUnits(SVGLengthType, float valueInSpecifiedUnits);
> 
> What does "new value" mean here?

I agree, we can use a better naming convention in SVGLengthValue -- after all only SVGLength is exposed to the Web, and we have to stick with the confusing official names like newValueSpecifiedUnits (see SVGLength.idl).

[Said Abou-Hallawa]

Created attachment 378596 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 378508 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        It used to allocate the least significant bits 4 bits to the SVGLengthMode
> 
> bits 4 bits -> 4 bits

Fixed.

>> Source/WebCore/svg/SVGLengthValue.cpp:328
>> +    if (from.lengthType() == lengthType() || from.isZero() || isZero() || from.lengthType() == SVGLengthType::EMS || from.lengthType() == SVGLengthType::EXS) {
> 
> This is pretty confusing.

I replaced the last two clauses "from.lengthType() == SVGLengthType::EMS || from.lengthType() == SVGLengthType::EXS" with from.isRelative() since from.lengthType() == SVGLengthType::Percentage is handled above..

[Said Abou-Hallawa]

Created attachment 378598 [details]
Patch

[Said Abou-Hallawa]

Created attachment 378599 [details]
Patch

[Said Abou-Hallawa]

Created attachment 378635 [details]
Patch

[Said Abou-Hallawa]

Created attachment 378682 [details]
Patch

[Said Abou-Hallawa]

Created attachment 378687 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 378687 [details]
Patch

Clearing flags on attachment: 378687

Committed r249822: <https://trac.webkit.org/changeset/249822>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55324552>