187258 2018-07-02 11:37:05 -0700 Introduce SVGAttribute 2018-08-06 14:26:53 -0700 1 1 1 Unclassified WebKit SVG WebKit Nightly Build Unspecified Unspecified RESOLVED DUPLICATE 186751 P2 Normal --- 186751 1 sabouhallawa sabouhallawa dbates dino jonlee koivisto krit rniwa simon.fraser thorton zimmermann oldest_to_newest 1438565 0 sabouhallawa 2018-07-02 11:37:05 -0700 Introduce the classes which are needed to redefine the SVGElements' attribute. This work is towards removing the SVG elements' attributes macros. 1438566 1 344120 sabouhallawa 2018-07-02 11:38:17 -0700 Created attachment 344120 Patch 1441091 2 344120 dbates 2018-07-10 21:23:33 -0700 Comment on attachment 344120 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344120&action=review > Source/WebCore/ChangeLog:8 > + This work is towards removing the SVG attributes macros. It is great that you explained your plan. We tend to prefer to patches that both add a new data structure and include at least one actual code change that makes use of the new data structure. The latter provides a concrete example of how the data structure is used and reduces the likelihood that we are leave dead code in the repository in the unlikely event that you are unable to follow through with your plan. > Source/WebCore/svg/properties/SVGAttribute.h:64 > +struct SVGPropertyAttribute : public SVGAttribute { > + SVGPropertyAttribute() > + : m_property(SVGPropertyTraits<PropertyType>::initialValue()) > + { > + } > + > + template<typename... Arguments> > + SVGPropertyAttribute(Arguments&&... arguments) > + : m_property(std::forward<Arguments>(arguments)...) > + { > + } > + > + PropertyType& value() { return m_property; } > + const PropertyType& value() const { return m_property; } > + > + void setValue(PropertyType&& property) { m_property = WTFMove(property); } > + void setValue(const PropertyType& property) { m_property = property; } > + void resetValue() { m_property = SVGPropertyTraits<PropertyType>::initialValue(); } > + > + String toString() const { return SVGPropertyTraits<PropertyType>::toString(m_property); } > + > + void setShouldSynchronize(bool shouldSynchronize) { m_shouldSynchronize = shouldSynchronize; } > + bool shouldSynchronize() const { return m_shouldSynchronize; } > + void synchronize(Element& element, const QualifiedName& attributeName) > + { > + if (!m_shouldSynchronize) > + return; > + element.setSynchronizedLazyAttribute(attributeName, toString()); > + } This is weird. We are underutilizing the purpose of the keyword "struct" and using it solely to avoid typing "public:\n". Having said that this data structure feels like a struct because it does not provide any encapsulation: it exposes all its instance data via setters and getters. > Source/WebCore/svg/properties/SVGAttribute.h:79 > + SVGAnimatedAttribute() > + { > + } = default? > Source/WebCore/svg/properties/SVGAttribute.h:96 > + SVGAnimatedAttributeList() > + { > + } = default? 1441199 3 344120 simon.fraser 2018-07-11 11:39:45 -0700 Comment on attachment 344120 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344120&action=review > Source/WebCore/svg/properties/SVGAttribute.h:33 > +struct SVGAttribute { }; "attribute" has a well-known meaning in the context of HTML/XML, and I wonder if using the term here is going to cause confusion. 1446423 4 sabouhallawa 2018-07-30 16:42:55 -0700 The plan is submit all the SVG macro removal change as one patch although reviewing it will be done through multiple incremental patches in 186751. *** This bug has been marked as a duplicate of bug 186751 *** 344120 2018-07-02 11:38:17 -0700 2018-08-06 14:26:53 -0700 Patch bug-187258-20180702113816.patch text/plain 6540 sabouhallawa SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDIzMzQyMykKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDQ1IEBACisyMDE4LTA3LTAyICBTYWlkIEFi b3UtSGFsbGF3YSAgPHNhYm91aGFsbGF3YUBhcHBsZS5jb20+CisKKyAgICAgICAgSW50cm9kdWNl IFNWR0F0dHJpYnV0ZQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9MTg3MjU4CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgVGhpcyB3b3JrIGlzIHRvd2FyZHMgcmVtb3ZpbmcgdGhlIFNWRyBhdHRyaWJ1dGVzIG1h Y3Jvcy4KKworICAgICAgICBTVkdBdHRyaWJ1dGUgaXMgdGhlIGJhc2UgY2xhc3MgZm9yIGEgc2V0 IG9mIGNsYXNzZXMgd2hpY2ggYXJlIG1lYW50IHRvIHRoZQorICAgICAgICB0eXBlcyBvZiBhbGwg dGhlIFNWR0VsZW1lbnRzJyBhdHRyaWJ1dGVzLiBUaGUgcGxhbiBpcyB0byBoYXZlIFNWR0F0dHJp YnV0ZQorICAgICAgICBSZWZDb3VudGVkIGFmdGVyIHRoZSBTVkcgYXR0cmlidXRlcyBtYWNyb3Mg YXJlIHJlbW92ZWQuCisKKyAgICAgICAgLS0gU1ZHUHJvcGVydHlBdHRyaWJ1dGUgcmVwcmVzZW50 cyBhIGhvbGRlciBmb3IgYSBwcm9wZXJ0eSB3aG9zZSB2YWx1ZSBjYW4KKyAgICAgICAgYmUgc3lu Y2hyb25pemVkIHdpdGggdGhlIEVsZW1lbnQuIEVsZW1lbnQ6OmdldEF0dHJpYnV0ZSgpIHdpbGwg Y2FsbAorICAgICAgICBTVkdQcm9wZXJ0eUF0dHJpYnV0ZTo6c3luY2hyb25pemUoKSB3aGljaCB3 aWxsIHNldCB0aGUgY2FsbCAKKyAgICAgICAgRWxlbWVudDo6c2V0QXR0cmlidXRlSW50ZXJuYWwo KSB3aXRoIHRoZSBsYXRlc3QgbW9kaWZpZWQgdmFsdWUuCisKKyAgICAgICAgLS0gU1ZHQW5pbWF0 ZWRBdHRyaWJ1dGUgcmVwcmVzZW50cyBhbiBTVkcgYW5pbWF0YWJsZSBhdHRyaWJ1dGUsIGUuZy4g CisgICAgICAgIHRoZSAneCcgYXR0cmlidXRlIG9mIHRoZSA8cmVjdD4gZWxlbWVudC4gQW4gU1ZH QW5pbWF0ZWRBdHRyaWJ1dGUgc2hvdWxkCisgICAgICAgIGhhdmUgdGhlIGFiaWxpdHkgdG8gY3Jl YXRlIGEgVGVhck9mZiBvYmplY3QgKHdpbGwgYmUgYWRkZWQgbGF0ZXIpIHdoaWNoCisgICAgICAg IHdpbGwgYmUgdXNlZCBhcyB0aGUgYmFja2luZ3MgdG9yZSBvZiB0aGUgRE9NIG9iamVjdCBhbmQg d2lsbCBiZSB1c2VkIGFsc28KKyAgICAgICAgdG8gaG9sZCB0aGUgYW5pbWF0ZWQgdmFsdWUgb2Yg dGhpcyBhdHRyaWJ1dGUgd2hpbGUgdGhlIGFuaW1hdGlvbiBpcyBpbgorICAgICAgICBwcm9ncmVz cy4KKworICAgICAgICAtLSBTVkdBbmltYXRlZEF0dHJpYnV0ZUxpc3QgcmVwcmVzZW50cyBhbiBT VkcgYW5pbWF0YWJsZSBsaXN0IGF0dHJpYnV0ZQorICAgICAgICBlLmcuIHRoZSAneCcgYXR0cmli dXRlIG9mIHRoZSA8dGV4dD4gZWxlbWVudC4gTW9yZSBsaXN0IHNwZWNpZmljIGZ1bmN0aW9ucwor ICAgICAgICB3aWxsIGJlIGFkZGVkIGxhdGVyLgorCisgICAgICAgICogc3ZnL3Byb3BlcnRpZXMv U1ZHQXR0cmlidXRlLmg6IEFkZGVkLgorICAgICAgICAoV2ViQ29yZTo6U1ZHUHJvcGVydHlBdHRy aWJ1dGU6OlNWR1Byb3BlcnR5QXR0cmlidXRlKToKKyAgICAgICAgKFdlYkNvcmU6OlNWR1Byb3Bl cnR5QXR0cmlidXRlOjp2YWx1ZSk6CisgICAgICAgIChXZWJDb3JlOjpTVkdQcm9wZXJ0eUF0dHJp YnV0ZTo6dmFsdWUgY29uc3QpOgorICAgICAgICAoV2ViQ29yZTo6U1ZHUHJvcGVydHlBdHRyaWJ1 dGU6OnNldFZhbHVlKToKKyAgICAgICAgKFdlYkNvcmU6OlNWR1Byb3BlcnR5QXR0cmlidXRlOjpy ZXNldFZhbHVlKToKKyAgICAgICAgKFdlYkNvcmU6OlNWR1Byb3BlcnR5QXR0cmlidXRlOjp0b1N0 cmluZyBjb25zdCk6CisgICAgICAgIChXZWJDb3JlOjpTVkdQcm9wZXJ0eUF0dHJpYnV0ZTo6c2V0 U2hvdWxkU3luY2hyb25pemUpOgorICAgICAgICAoV2ViQ29yZTo6U1ZHUHJvcGVydHlBdHRyaWJ1 dGU6OnNob3VsZFN5bmNocm9uaXplIGNvbnN0KToKKyAgICAgICAgKFdlYkNvcmU6OlNWR1Byb3Bl cnR5QXR0cmlidXRlOjpzeW5jaHJvbml6ZSk6CisgICAgICAgIChXZWJDb3JlOjpTVkdBbmltYXRl ZEF0dHJpYnV0ZTo6U1ZHQW5pbWF0ZWRBdHRyaWJ1dGUpOgorICAgICAgICAoV2ViQ29yZTo6U1ZH QW5pbWF0ZWRBdHRyaWJ1dGVMaXN0OjpTVkdBbmltYXRlZEF0dHJpYnV0ZUxpc3QpOgorCiAyMDE4 LTA3LTAxICBGdWppaSBIaXJvbm9yaSAgPEhpcm9ub3JpLkZ1amlpQHNvbnkuY29tPgogCiAgICAg ICAgIFtXSzJdIGZhc3QvcGFyc2VyL2RvY3VtZW50LW9wZW4taW4tdW5sb2FkLmh0bWwgbWFrZXMg dGhlIGZvbGxvd2luZyB0ZXN0IGNyYXNoCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9zdmcvcHJvcGVy dGllcy9TVkdBdHRyaWJ1dGUuaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9zdmcvcHJv cGVydGllcy9TVkdBdHRyaWJ1dGUuaAkobm9uZXhpc3RlbnQpCisrKyBTb3VyY2UvV2ViQ29yZS9z dmcvcHJvcGVydGllcy9TVkdBdHRyaWJ1dGUuaAkod29ya2luZyBjb3B5KQpAQCAtMCwwICsxLDEw NSBAQAorLyoKKyAqIENvcHlyaWdodCAoQykgMjAxOCBBcHBsZSBJbmMuICBBbGwgcmlnaHRzIHJl c2VydmVkLgorICoKKyAqIFJlZGlzdHJpYnV0aW9uIGFuZCB1c2UgaW4gc291cmNlIGFuZCBiaW5h cnkgZm9ybXMsIHdpdGggb3Igd2l0aG91dAorICogbW9kaWZpY2F0aW9uLCBhcmUgcGVybWl0dGVk IHByb3ZpZGVkIHRoYXQgdGhlIGZvbGxvd2luZyBjb25kaXRpb25zCisgKiBhcmUgbWV0OgorICog MS4gUmVkaXN0cmlidXRpb25zIG9mIHNvdXJjZSBjb2RlIG11c3QgcmV0YWluIHRoZSBhYm92ZSBj b3B5cmlnaHQKKyAqICAgIG5vdGljZSwgdGhpcyBsaXN0IG9mIGNvbmRpdGlvbnMgYW5kIHRoZSBm b2xsb3dpbmcgZGlzY2xhaW1lci4KKyAqIDIuIFJlZGlzdHJpYnV0aW9ucyBpbiBiaW5hcnkgZm9y bSBtdXN0IHJlcHJvZHVjZSB0aGUgYWJvdmUgY29weXJpZ2h0CisgKiAgICBub3RpY2UsIHRoaXMg bGlzdCBvZiBjb25kaXRpb25zIGFuZCB0aGUgZm9sbG93aW5nIGRpc2NsYWltZXIgaW4gdGhlCisg KiAgICBkb2N1bWVudGF0aW9uIGFuZC9vciBvdGhlciBtYXRlcmlhbHMgcHJvdmlkZWQgd2l0aCB0 aGUgZGlzdHJpYnV0aW9uLgorICoKKyAqIFRISVMgU09GVFdBUkUgSVMgUFJPVklERUQgQlkgQVBQ TEUgSU5DLiBgYEFTIElTJycgQU5EIEFOWQorICogRVhQUkVTUyBPUiBJTVBMSUVEIFdBUlJBTlRJ RVMsIElOQ0xVRElORywgQlVUIE5PVCBMSU1JVEVEIFRPLCBUSEUKKyAqIElNUExJRUQgV0FSUkFO VElFUyBPRiBNRVJDSEFOVEFCSUxJVFkgQU5EIEZJVE5FU1MgRk9SIEEgUEFSVElDVUxBUgorICog UFVSUE9TRSBBUkUgRElTQ0xBSU1FRC4gIElOIE5PIEVWRU5UIFNIQUxMIEFQUExFIElOQy4gT1IK KyAqIENPTlRSSUJVVE9SUyBCRSBMSUFCTEUgRk9SIEFOWSBESVJFQ1QsIElORElSRUNULCBJTkNJ REVOVEFMLCBTUEVDSUFMLAorICogRVhFTVBMQVJZLCBPUiBDT05TRVFVRU5USUFMIERBTUFHRVMg KElOQ0xVRElORywgQlVUIE5PVCBMSU1JVEVEIFRPLAorICogUFJPQ1VSRU1FTlQgT0YgU1VCU1RJ VFVURSBHT09EUyBPUiBTRVJWSUNFUzsgTE9TUyBPRiBVU0UsIERBVEEsIE9SCisgKiBQUk9GSVRT OyBPUiBCVVNJTkVTUyBJTlRFUlJVUFRJT04pIEhPV0VWRVIgQ0FVU0VEIEFORCBPTiBBTlkgVEhF T1JZCisgKiBPRiBMSUFCSUxJVFksIFdIRVRIRVIgSU4gQ09OVFJBQ1QsIFNUUklDVCBMSUFCSUxJ VFksIE9SIFRPUlQKKyAqIChJTkNMVURJTkcgTkVHTElHRU5DRSBPUiBPVEhFUldJU0UpIEFSSVNJ TkcgSU4gQU5ZIFdBWSBPVVQgT0YgVEhFIFVTRQorICogT0YgVEhJUyBTT0ZUV0FSRSwgRVZFTiBJ RiBBRFZJU0VEIE9GIFRIRSBQT1NTSUJJTElUWSBPRiBTVUNIIERBTUFHRS4KKyAqLworCisjcHJh Z21hIG9uY2UKKworI2luY2x1ZGUgIkVsZW1lbnQuaCIKKyNpbmNsdWRlICJTVkdQcm9wZXJ0eVRy YWl0cy5oIgorCituYW1lc3BhY2UgV2ViQ29yZSB7CisKK3N0cnVjdCBTVkdBdHRyaWJ1dGUgeyB9 OworCit0ZW1wbGF0ZTx0eXBlbmFtZSBQcm9wZXJ0eVR5cGU+CitzdHJ1Y3QgU1ZHUHJvcGVydHlB dHRyaWJ1dGUgOiBwdWJsaWMgU1ZHQXR0cmlidXRlIHsKKyAgICBTVkdQcm9wZXJ0eUF0dHJpYnV0 ZSgpCisgICAgICAgIDogbV9wcm9wZXJ0eShTVkdQcm9wZXJ0eVRyYWl0czxQcm9wZXJ0eVR5cGU+ Ojppbml0aWFsVmFsdWUoKSkKKyAgICB7CisgICAgfQorCisgICAgdGVtcGxhdGU8dHlwZW5hbWUu Li4gQXJndW1lbnRzPgorICAgIFNWR1Byb3BlcnR5QXR0cmlidXRlKEFyZ3VtZW50cyYmLi4uIGFy Z3VtZW50cykKKyAgICAgICAgOiBtX3Byb3BlcnR5KHN0ZDo6Zm9yd2FyZDxBcmd1bWVudHM+KGFy Z3VtZW50cykuLi4pCisgICAgeworICAgIH0KKworICAgIFByb3BlcnR5VHlwZSYgdmFsdWUoKSB7 IHJldHVybiBtX3Byb3BlcnR5OyB9CisgICAgY29uc3QgUHJvcGVydHlUeXBlJiB2YWx1ZSgpIGNv bnN0IHsgcmV0dXJuIG1fcHJvcGVydHk7IH0KKworICAgIHZvaWQgc2V0VmFsdWUoUHJvcGVydHlU eXBlJiYgcHJvcGVydHkpIHsgbV9wcm9wZXJ0eSA9IFdURk1vdmUocHJvcGVydHkpOyB9CisgICAg dm9pZCBzZXRWYWx1ZShjb25zdCBQcm9wZXJ0eVR5cGUmIHByb3BlcnR5KSB7IG1fcHJvcGVydHkg PSBwcm9wZXJ0eTsgfQorICAgIHZvaWQgcmVzZXRWYWx1ZSgpIHsgbV9wcm9wZXJ0eSA9IFNWR1By b3BlcnR5VHJhaXRzPFByb3BlcnR5VHlwZT46OmluaXRpYWxWYWx1ZSgpOyB9CisKKyAgICBTdHJp bmcgdG9TdHJpbmcoKSBjb25zdCB7IHJldHVybiBTVkdQcm9wZXJ0eVRyYWl0czxQcm9wZXJ0eVR5 cGU+Ojp0b1N0cmluZyhtX3Byb3BlcnR5KTsgfQorCisgICAgdm9pZCBzZXRTaG91bGRTeW5jaHJv bml6ZShib29sIHNob3VsZFN5bmNocm9uaXplKSB7IG1fc2hvdWxkU3luY2hyb25pemUgPSBzaG91 bGRTeW5jaHJvbml6ZTsgfQorICAgIGJvb2wgc2hvdWxkU3luY2hyb25pemUoKSBjb25zdCB7IHJl dHVybiBtX3Nob3VsZFN5bmNocm9uaXplOyB9CisgICAgdm9pZCBzeW5jaHJvbml6ZShFbGVtZW50 JiBlbGVtZW50LCBjb25zdCBRdWFsaWZpZWROYW1lJiBhdHRyaWJ1dGVOYW1lKQorICAgIHsKKyAg ICAgICAgaWYgKCFtX3Nob3VsZFN5bmNocm9uaXplKQorICAgICAgICAgICAgcmV0dXJuOworICAg ICAgICBlbGVtZW50LnNldFN5bmNocm9uaXplZExhenlBdHRyaWJ1dGUoYXR0cmlidXRlTmFtZSwg dG9TdHJpbmcoKSk7CisgICAgfQorCitwcm90ZWN0ZWQ6CisgICAgUHJvcGVydHlUeXBlIG1fcHJv cGVydHk7CisgICAgYm9vbCBtX3Nob3VsZFN5bmNocm9uaXplIHsgZmFsc2UgfTsKK307CisKK3Rl bXBsYXRlPHR5cGVuYW1lIFRlYXJPZmZUeXBlPgorc3RydWN0IFNWR0FuaW1hdGVkQXR0cmlidXRl IDogcHVibGljIFNWR1Byb3BlcnR5QXR0cmlidXRlPHR5cGVuYW1lIFRlYXJPZmZUeXBlOjpDb250 ZW50VHlwZT4geworICAgIHVzaW5nIFByb3BlcnR5VGVhck9mZlR5cGUgPSBUZWFyT2ZmVHlwZTsK KyAgICB1c2luZyBQcm9wZXJ0eVR5cGUgPSB0eXBlbmFtZSBQcm9wZXJ0eVRlYXJPZmZUeXBlOjpD b250ZW50VHlwZTsKKyAgICB1c2luZyBCYXNlID0gU1ZHUHJvcGVydHlBdHRyaWJ1dGU8UHJvcGVy dHlUeXBlPjsKKworICAgIFNWR0FuaW1hdGVkQXR0cmlidXRlKCkKKyAgICB7CisgICAgfQorCisg ICAgdGVtcGxhdGU8dHlwZW5hbWUuLi4gQXJndW1lbnRzPgorICAgIFNWR0FuaW1hdGVkQXR0cmli dXRlKEFyZ3VtZW50cyYmLi4uIGFyZ3VtZW50cykKKyAgICAgICAgOiBCYXNlKHN0ZDo6Zm9yd2Fy ZDxBcmd1bWVudHM+KGFyZ3VtZW50cykuLi4pCisgICAgeworICAgIH0KK307CisKK3RlbXBsYXRl PHR5cGVuYW1lIFRlYXJPZmZUeXBlPgorc3RydWN0IFNWR0FuaW1hdGVkQXR0cmlidXRlTGlzdCA6 IHB1YmxpYyBTVkdBbmltYXRlZEF0dHJpYnV0ZTxUZWFyT2ZmVHlwZT4geworICAgIHVzaW5nIFBy b3BlcnR5VGVhck9mZlR5cGUgPSBUZWFyT2ZmVHlwZTsKKyAgICB1c2luZyBQcm9wZXJ0eVR5cGUg PSB0eXBlbmFtZSBQcm9wZXJ0eVRlYXJPZmZUeXBlOjpDb250ZW50VHlwZTsKKyAgICB1c2luZyBC YXNlID0gU1ZHQW5pbWF0ZWRBdHRyaWJ1dGU8UHJvcGVydHlUZWFyT2ZmVHlwZT47CisKKyAgICBT VkdBbmltYXRlZEF0dHJpYnV0ZUxpc3QoKQorICAgIHsKKyAgICB9CisKKyAgICB0ZW1wbGF0ZTx0 eXBlbmFtZS4uLiBBcmd1bWVudHM+CisgICAgU1ZHQW5pbWF0ZWRBdHRyaWJ1dGVMaXN0KEFyZ3Vt ZW50cyYmLi4uIGFyZ3VtZW50cykKKyAgICAgICAgOiBCYXNlKHN0ZDo6Zm9yd2FyZDxBcmd1bWVu dHM+KGFyZ3VtZW50cykuLi4pCisgICAgeworICAgIH0KK307CisKK30K