RESOLVED FIXED 201663
SVGLengthValue should use two enums for 'type' and 'mode' instead of one unsigned for 'units' 
https://bugs.webkit.org/show_bug.cgi?id=201663
Summary SVGLengthValue should use two enums for 'type' and 'mode' instead of one unsi...
Said Abou-Hallawa
Reported 2019-09-10 15:59:19 PDT
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.
Attachments
Patch (126.68 KB, patch)
2019-09-10 16:25 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (131.65 KB, patch)
2019-09-11 16:39 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (131.63 KB, patch)
2019-09-11 16:54 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (131.63 KB, patch)
2019-09-11 16:58 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (131.88 KB, patch)
2019-09-12 01:47 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (128.37 KB, patch)
2019-09-12 15:28 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (128.47 KB, patch)
2019-09-12 16:17 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.
Said Abou-Hallawa
Comment 1 2019-09-10 16:25:05 PDT
Created attachment 378508 [details] Patch
Simon Fraser (smfr)
Comment 2 2019-09-10 17:31:12 PDT
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 3 2019-09-11 01:42:01 PDT
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
Comment 4 2019-09-11 16:39:55 PDT
Created attachment 378596 [details] Patch
Said Abou-Hallawa
Comment 5 2019-09-11 16:53:26 PDT
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
Comment 6 2019-09-11 16:54:31 PDT
Created attachment 378598 [details] Patch
Said Abou-Hallawa
Comment 7 2019-09-11 16:58:50 PDT
Created attachment 378599 [details] Patch
Said Abou-Hallawa
Comment 8 2019-09-12 01:47:20 PDT
Created attachment 378635 [details] Patch
Said Abou-Hallawa
Comment 9 2019-09-12 15:28:10 PDT
Created attachment 378682 [details] Patch
Said Abou-Hallawa
Comment 10 2019-09-12 16:17:14 PDT
Created attachment 378687 [details] Patch
WebKit Commit Bot
Comment 11 2019-09-12 17:40:02 PDT
Comment on attachment 378687 [details] Patch Clearing flags on attachment: 378687 Committed r249822: <https://trac.webkit.org/changeset/249822>
WebKit Commit Bot
Comment 12 2019-09-12 17:40:04 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 13 2019-09-12 17:41:19 PDT
<rdar://problem/55324552>
Note You need to log in before you can comment on or make changes to this bug.