Bug 201663

Summary:

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

Product:

WebKit

Reporter:

Said Abou-Hallawa <sabouhallawa>

Component:

SVG

Assignee:

Said Abou-Hallawa <sabouhallawa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

allan.jensen, cdumez, commit-queue, dino, dstockwell, esprehn+autocc, ews-watchlist, fmalita, glenn, gyuyoung.kim, kondapallykalyan, macpherson, menard, pdr, schenney, sergio, simon.fraser, webkit-bug-importer, zimmermann

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

199843

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

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	Details Formatted Diff Diff
Patch (131.65 KB, patch) 2019-09-11 16:39 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch (131.63 KB, patch) 2019-09-11 16:54 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch (131.63 KB, patch) 2019-09-11 16:58 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch (131.88 KB, patch) 2019-09-12 01:47 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch (128.37 KB, patch) 2019-09-12 15:28 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch (128.47 KB, patch) 2019-09-12 16:17 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
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