Bug 168586

Created attachment 302119 [details] Patch

Comment on attachment 302119 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302119&action=review > Source/WebCore/svg/SVGAnimatedType.h:33 > + return std::unique_ptr<SVGAnimatedType>(new SVGAnimatedType(SVGPropertyTraits<PropertyType>::initialValue())); Can this use make_unique to avoid the redundant type name. > Source/WebCore/svg/SVGAnimatedType.h:39 > + return std::unique_ptr<SVGAnimatedType>(new SVGAnimatedType(WTFMove(property))); make_unique? > Source/WebCore/svg/SVGAnimatedType.h:50 > + return std::unique_ptr<SVGAnimatedType>(new SVGAnimatedType(SVGPropertyTraits<std::pair<PropertyType1, PropertyType2>>::initialValue())); make_unique? (I'll stop repeating now). > Source/WebCore/svg/SVGAnimatedType.h:79 > + return std::pair<PropertyType1&, PropertyType2&>(m_values[0]->property<PropertyType1>(), m_values[1]->property<PropertyType2>()); Would be good to have some assertion that m_values[1] is not null. > Source/WebCore/svg/SVGAnimatedType.h:112 > + RefPtr<SVGAnimatedValue> m_values[2]; What's the value of making this an array rather than just m_value1, and m_value2? > Source/WebCore/svg/SVGAnimatedValue.h:41 > +#include "SVGTransformListValues.h" > + > +#include <wtf/Variant.h> No reason to separate these. > Source/WebCore/svg/SVGAnimatedValue.h:64 > + public WTF::RefCounted<SVGAnimatedValue>, WTF:: not needed. Should all be on one line. > Source/WebCore/svg/SVGAnimatedValue.h:68 > + static RefPtr<SVGAnimatedValue> create(const PropertyType& value) Can this return a Ref<>? > Source/WebCore/svg/SVGAnimatedValue.h:74 > + static RefPtr<SVGAnimatedValue> create(PropertyType&& value) Ditto. > Source/WebCore/svg/SVGAnimatedValue.h:80 > + static RefPtr<SVGAnimatedValue> create(const SVGAnimatedValue& value) Ditto. > Source/WebCore/svg/SVGAnimatedValue.h:86 > + template<typename PropertyType> > + bool holdsAlternative() const Despite it being the name chosen by the variant class, I don't think holdsAlternative is that great a name. Perhaps just good old is<>? > Source/WebCore/svg/SVGPathByteStream.h:26 > +#include "SVGPropertyTraits.h" > + > #include <wtf/Noncopyable.h> No need to separate these. > Source/WebCore/svg/SVGPathByteStream.h:49 > +class SVGPathByteStream final : public Vector<unsigned char, 1> { > WTF_MAKE_FAST_ALLOCATED; > public: I'm generally not a fan of using inheritance for things like this, as it tends to expose much more API surface area than necessary. I'd prefer to see this as kept as composition. > Source/WebCore/svg/properties/SVGPropertyTraits.h:29 > +#include "SVGParserUtilities.h" > + > +#include <wtf/text/StringBuilder.h> No need to separate these. > Source/WebCore/svg/properties/SVGPropertyTraits.h:78 > + return std::pair<int, int>(static_cast<int>(roundf(firstNumber)), static_cast<int>(roundf(secondNumber))); Will make_pair work here? > Source/WebCore/svg/properties/SVGPropertyTraits.h:146 > + static bool parse(String& type, const QualifiedName&, const String& string) > + { > + type = string; > + return true; > + } These parse methods should be converted to return optional<Foo> at some point.

Created attachment 302319 [details] Patch

Comment on attachment 302119 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302119&action=review >> Source/WebCore/svg/SVGAnimatedType.h:33 >> + return std::unique_ptr<SVGAnimatedType>(new SVGAnimatedType(SVGPropertyTraits<PropertyType>::initialValue())); > > Can this use make_unique to avoid the redundant type name. Done. What made me use unique_ptr is I wanted to have the constructors to be private. make_unique requires the constructor to be public or it is declared to be a friend to the class. Since there are many create() functions it was not worthy adding these friend functions. >> Source/WebCore/svg/SVGAnimatedType.h:39 >> + return std::unique_ptr<SVGAnimatedType>(new SVGAnimatedType(WTFMove(property))); > > make_unique? Done. >> Source/WebCore/svg/SVGAnimatedType.h:50 >> + return std::unique_ptr<SVGAnimatedType>(new SVGAnimatedType(SVGPropertyTraits<std::pair<PropertyType1, PropertyType2>>::initialValue())); > > make_unique? (I'll stop repeating now). Done. All the constructors were changed to be public and all the create() functions were changed to use make_unique. >> Source/WebCore/svg/SVGAnimatedType.h:79 >> + return std::pair<PropertyType1&, PropertyType2&>(m_values[0]->property<PropertyType1>(), m_values[1]->property<PropertyType2>()); > > Would be good to have some assertion that m_values[1] is not null. Done. >> Source/WebCore/svg/SVGAnimatedType.h:112 >> + RefPtr<SVGAnimatedValue> m_values[2]; > > What's the value of making this an array rather than just m_value1, and m_value2? Done. >> Source/WebCore/svg/SVGAnimatedValue.h:41 >> +#include <wtf/Variant.h> > > No reason to separate these. Empty line was removed. >> Source/WebCore/svg/SVGAnimatedValue.h:64 >> + public WTF::RefCounted<SVGAnimatedValue>, > > WTF:: not needed. Should all be on one line. WTF:: was removed and all were made on one line. >> Source/WebCore/svg/SVGAnimatedValue.h:68 >> + static RefPtr<SVGAnimatedValue> create(const PropertyType& value) > > Can this return a Ref<>? Done. >> Source/WebCore/svg/SVGAnimatedValue.h:74 >> + static RefPtr<SVGAnimatedValue> create(PropertyType&& value) > > Ditto. Done. >> Source/WebCore/svg/SVGAnimatedValue.h:80 >> + static RefPtr<SVGAnimatedValue> create(const SVGAnimatedValue& value) > > Ditto. Done. >> Source/WebCore/svg/SVGAnimatedValue.h:86 >> + bool holdsAlternative() const > > Despite it being the name chosen by the variant class, I don't think holdsAlternative is that great a name. Perhaps just good old is<>? Function was renamed to be is<>(). >> Source/WebCore/svg/SVGPathByteStream.h:26 >> #include <wtf/Noncopyable.h> > > No need to separate these. Empty line was removed. >> Source/WebCore/svg/SVGPathByteStream.h:49 >> public: > > I'm generally not a fan of using inheritance for things like this, as it tends to expose much more API surface area than necessary. I'd prefer to see this as kept as composition. Done. What convince me to do it this way was SVGLengthListValues is derived from Vector<SVGLengthValue> and SVGPointListValues is derived Vector<FloatPoint>. >> Source/WebCore/svg/properties/SVGPropertyTraits.h:29 >> +#include <wtf/text/StringBuilder.h> > > No need to separate these. Empty line was removed. >> Source/WebCore/svg/properties/SVGPropertyTraits.h:78 >> + return std::pair<int, int>(static_cast<int>(roundf(firstNumber)), static_cast<int>(roundf(secondNumber))); > > Will make_pair work here? Done. >> Source/WebCore/svg/properties/SVGPropertyTraits.h:146 >> + } > > These parse methods should be converted to return optional<Foo> at some point. All SVGPropertyTraits<Foo>::parse() functions were changed to return std::optional<Foo>. SVGAnimatedValue::parse() was changed to check the returned std::optional<>.

Created attachment 302332 [details] Patch

Created attachment 333616 [details] Patch

Created attachment 333621 [details] Patch

Attachment 333621 [details] did not pass style-queue: ERROR: Source/WebCore/svg/SVGValue.h:49: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:50: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:51: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:52: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:53: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:54: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:55: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:56: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:57: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:58: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:59: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:60: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:61: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:62: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:63: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:64: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:65: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:66: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:67: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:68: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:69: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:81: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/svg/SVGValue.h:108: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/ChangeLog:16: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free, use-after-free [changelog/unwantedsecurityterms] [3] Total errors found: 24 in 110 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 333629 [details] Patch

Attachment 333629 [details] did not pass style-queue: ERROR: Source/WebCore/svg/SVGValue.h:49: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:50: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:51: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:52: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:53: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:54: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:55: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:56: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:57: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:58: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:59: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:60: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:61: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:62: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:63: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:64: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:65: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:66: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:67: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:68: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:69: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:81: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/svg/SVGValue.h:108: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/ChangeLog:16: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free, use-after-free [changelog/unwantedsecurityterms] [3] Total errors found: 24 in 110 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 333638 [details] Patch

Attachment 333638 [details] did not pass style-queue: ERROR: Source/WebCore/svg/SVGValue.h:49: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:50: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:51: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:52: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:53: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:54: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:55: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:56: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:57: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:58: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:59: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:60: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:61: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:62: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:63: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:64: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:65: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:66: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:67: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:68: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:69: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:81: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/svg/SVGValue.h:108: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/ChangeLog:16: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free, use-after-free [changelog/unwantedsecurityterms] [3] Total errors found: 24 in 110 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 333646 [details] Patch

Attachment 333646 [details] did not pass style-queue: ERROR: Source/WebCore/svg/SVGValue.h:49: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:50: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:51: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:52: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:53: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:54: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:55: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:56: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:57: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:58: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:59: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:60: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:61: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:62: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:63: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:64: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:65: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:66: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:67: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:68: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:69: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:81: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/svg/SVGValue.h:108: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/ChangeLog:16: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free, use-after-free [changelog/unwantedsecurityterms] [3] Total errors found: 24 in 110 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 333728 [details] Patch

Attachment 333728 [details] did not pass style-queue: ERROR: Source/WebCore/svg/SVGValue.h:49: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:50: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:51: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:52: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:53: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:54: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:55: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:56: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:57: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:58: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:59: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:60: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:61: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:62: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:63: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:64: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:65: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:66: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:67: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:68: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:69: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:81: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/svg/SVGValue.h:108: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/ChangeLog:16: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free, use-after-free [changelog/unwantedsecurityterms] [3] Total errors found: 24 in 111 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 333814 [details] Patch

Attachment 333814 [details] did not pass style-queue: ERROR: Source/WebCore/svg/SVGValue.h:49: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:50: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:51: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:52: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:53: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:54: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:55: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:56: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:57: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:58: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:59: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:60: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:61: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:62: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:63: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:64: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:65: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:66: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:67: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:68: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:69: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:81: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/svg/SVGValue.h:108: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/ChangeLog:16: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free, use-after-free [changelog/unwantedsecurityterms] [3] Total errors found: 24 in 111 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 333814 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333814&action=review > Source/WebCore/ChangeLog:3 > + Make all the SVGElements' attributes be RefCounted objects This is a crazy big patch. I wonder if you can factor out *at least* the “fix header dependency problems” bits, and maybe the toString/setValueAsString renames? > Source/WebCore/ChangeLog:26 > + Creating different classes for every data type an SVG attribute does not > + look the cleanest solution. Besides, there are cases where the union First sentence here has a grammar problem. > Source/WebCore/ChangeLog:35 > + -- Change SVGAnimatedType to hold two SVGValues. The second one is optional > + and only used for animated types which requires two data types. Still not 100% sure why we don’t just make a new type (it can probably even be an alias for std::pair) for the two types that need two values, and just use SVGValue for everything? > Source/WebCore/svg/SVGNumberListValues.h:35 > + :) > Source/WebCore/svg/SVGParserUtilities.cpp:243 > + // disallow anything except spaces at the end Comments should be sentences with capitals and punctuation. > Source/WebCore/svg/SVGValue.h:153 > + return adoptRef(*new SVGValue(reinterpret_cast<const unsigned&>(property))); What if the enum is bigger than unsigned? Can we make everyone use enum class and use the real type?

Created attachment 333931 [details] Patch

Attachment 333931 [details] did not pass style-queue: ERROR: Source/WebCore/svg/SVGValue.h:49: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:50: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:51: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:52: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:53: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:54: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:55: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:56: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:57: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:58: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:59: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:60: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:61: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:62: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:63: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:64: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:65: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:66: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:67: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:68: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:69: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:81: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/svg/SVGValue.h:108: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/ChangeLog:16: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free, use-after-free [changelog/unwantedsecurityterms] [3] Total errors found: 24 in 108 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 333814 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333814&action=review >> Source/WebCore/ChangeLog:3 >> + Make all the SVGElements' attributes be RefCounted objects > > This is a crazy big patch. I wonder if you can factor out *at least* the “fix header dependency problems” bits, and maybe the toString/setValueAsString renames? I tried by reming some of the re-factoring but I do not think this helped much reducing the size of the patch. >> Source/WebCore/ChangeLog:26 >> + look the cleanest solution. Besides, there are cases where the union > > First sentence here has a grammar problem. Fixed. >> Source/WebCore/ChangeLog:35 >> + and only used for animated types which requires two data types. > > Still not 100% sure why we don’t just make a new type (it can probably even be an alias for std::pair) for the two types that need two values, and just use SVGValue for everything? Please have a look at SVGAnimatedTypeAnimator::constructFromBaseValues() and notice that we have to call executeAction(StartAnimationAction, ...) twice one for each type. Every time we have to pass a separate SVGValue since this is going to be attached to a separate tear-off object. If the SVGValue has an alternative like std::pair<SVGAngleValue,SVGMarkerOrientType> for example, we will not be able to split it into RefCounted object each is referenced by a tear-off object. >> Source/WebCore/svg/SVGNumberListValues.h:35 >> + > > :) Space was removed. >> Source/WebCore/svg/SVGParserUtilities.cpp:243 >> + // disallow anything except spaces at the end > > Comments should be sentences with capitals and punctuation. Fixed. >> Source/WebCore/svg/SVGValue.h:153 >> + return adoptRef(*new SVGValue(reinterpret_cast<const unsigned&>(property))); > > What if the enum is bigger than unsigned? Can we make everyone use enum class and use the real type? Please look at SVGAnimatedEnumerationPropertyTearOff and search the code for highestEnumValue() and you will see that we already have the assumption that highestEnumValue() of any SVG enum has to fit inside unsigned.

Created attachment 333938 [details] Patch

Attachment 333938 [details] did not pass style-queue: ERROR: Source/WebCore/svg/SVGValue.h:49: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:50: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:51: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:52: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:53: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:54: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:55: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:56: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:57: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:58: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:59: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:60: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:61: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:62: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:63: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:64: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:65: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:66: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:67: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:68: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:69: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/svg/SVGValue.h:81: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/svg/SVGValue.h:108: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/ChangeLog:16: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free, use-after-free [changelog/unwantedsecurityterms] [3] Total errors found: 24 in 108 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 333961 [details] Patch

Comment on attachment 333814 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333814&action=review Can you add SVGType in a separate patch, without hooking it up to anything? Some early comments. >> Source/WebCore/ChangeLog:3 >> + Make all the SVGElements' attributes be RefCounted objects > > This is a crazy big patch. I wonder if you can factor out *at least* the “fix header dependency problems” bits, and maybe the toString/setValueAsString renames? Yeah. It would be nice to split out the robotic changes from the real code changes. > Source/WebCore/ChangeLog:16 > + was relaxed by using WeakPtr or raw pointer from one side to the other, a > + use-after-free bugs were happening. A more subtle problem can happen when s/a/some/ > Source/WebCore/ChangeLog:46 > + Add new files for SVGValue and SVGListVlaues. Move definitions from Typo: SVGListValues > Source/WebCore/ChangeLog:81 > + Don't pass SVGAnimatedType getter to resetFromBaseValues(). The PropertyTYpe Typo: PropertyType > Source/WebCore/svg/SVGAnimatedAngle.cpp:69 > + const auto& fromAngle = static_cast<SVGAngleValue&>(*from->values[0]); > + const auto& fromEnumeration = static_cast<unsigned&>(*from->values[1]); I don't like static_cast from SVGValue to underlying types of variants. It isn't really just a cast, since it is calling into a function that is checking the type and ASSERTing, etc. I'd prefer .asSVGAngleValue() or something, or maybe even a new asVariantType<SVGAngleValue> helper. > Source/WebCore/svg/SVGAnimatedAngle.cpp:86 > + auto& toAngle = static_cast<SVGAngleValue&>(*to->values[0]); > + auto& toEnumeration = static_cast<unsigned&>(*to->values[1]); Aren't these const too? > Source/WebCore/svg/SVGAnimatedColor.cpp:48 > + auto& fromColor = static_cast<Color&>(*from->values[0]); > + auto& toColor = static_cast<Color&>(*to->values[0]); Yeah, I think we need a better way to convert from SVGValue to the underlying types. > Source/WebCore/svg/SVGAnimatedNumberOptionalNumber.cpp:79 > + const auto& formFirst = m_animationElement->animationMode() == ToAnimation ? static_cast<float&>(*animated->values[0]) : static_cast<float&>(*from->values[0]); > + const auto& formSecond = m_animationElement->animationMode() == ToAnimation ? static_cast<float&>(*animated->values[1]) : static_cast<float&>(*from->values[1]); Typo: from > Source/WebCore/svg/SVGAnimatedNumberOptionalNumber.cpp:88 > + m_animationElement->animateAdditiveNumber(percentage, repeatCount, formFirst, toFirst, toAtEndOfDurationFirst, animatedFirst); > + m_animationElement->animateAdditiveNumber(percentage, repeatCount, formSecond, toSecond, toAtEndOfDurationSecond, animatedSecond); Typo: from > Source/WebCore/svg/SVGAnimatedType.h:57 > + template<typename PropertyType> > + static std::unique_ptr<SVGAnimatedType> create(PropertyType&& property) I don't understand why these are public but the constructor is also public. Which should people use? Can the actual constructor be private? > Source/WebCore/svg/SVGAnimatedType.h:107 > + ASSERT(values[0] && !values[1]); Why is it an error to call toString on something with two values? > Source/WebCore/svg/SVGAnimatedType.h:141 > + [](const SVGAngleValue&) { return AnimatedAngle; }, > + [](const bool&) { return AnimatedBoolean; }, > + [](const Color&) { return AnimatedColor; }, > + [](const unsigned&) { return AnimatedEnumeration; }, > + [](const int&) { return AnimatedInteger; }, > + [](const SVGLengthValue&) { return AnimatedLength; }, > + [](const SVGLengthListValues&) { return AnimatedLengthList; }, > + [](const float) { return AnimatedNumber; }, > + [](const SVGNumberListValues&) { return AnimatedNumberList; }, > + [](const SVGPathByteStream&) { return AnimatedPath; }, > + [](const SVGPreserveAspectRatioValue&) { return AnimatedPreserveAspectRatio; }, > + [](const SVGPointListValues&) { return AnimatedPoints; }, > + [](const FloatRect&) { return AnimatedRect; }, > + [](const String&) { return AnimatedString; }, > + [](const SVGTransformListValues&) { return AnimatedTransformList; }, > + [](const auto&) { return AnimatedUnknown; } I don't think we usually indent things this way. > Source/WebCore/svg/SVGAnimatedType.h:145 > + RefPtr<SVGValue> values[2]; Rather than have an array, why not separate first and second members? RefPtr<SVGValue> first; RefPtr<SVGValue> second; > Source/WebCore/svg/SVGFEConvolveMatrixElement.cpp:285 > for (int i = 0; i < kernelMatrixSize; ++i) > - divisorValue += kernelMatrix.at(i); > + divisorValue += kernelMatrix.propertyAt(i); Can you use a for : loop here?

Created attachment 334060 [details] Patch

Comment on attachment 333814 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333814&action=review >> Source/WebCore/ChangeLog:16 >> + use-after-free bugs were happening. A more subtle problem can happen when > > s/a/some/ Fixed. >> Source/WebCore/ChangeLog:46 >> + Add new files for SVGValue and SVGListVlaues. Move definitions from > > Typo: SVGListValues Fixed. >> Source/WebCore/ChangeLog:81 >> + Don't pass SVGAnimatedType getter to resetFromBaseValues(). The PropertyTYpe > > Typo: PropertyType Fixed. >> Source/WebCore/svg/SVGAnimatedAngle.cpp:69 >> + const auto& fromEnumeration = static_cast<unsigned&>(*from->values[1]); > > I don't like static_cast from SVGValue to underlying types of variants. It isn't really just a cast, since it is calling into a function that is checking the type and ASSERTing, etc. > > I'd prefer .asSVGAngleValue() or something, or maybe even a new asVariantType<SVGAngleValue> helper. I added a template function SVGValue::as() and I replaced all the casting by calls to as<PropertyType>(). >> Source/WebCore/svg/SVGAnimatedAngle.cpp:86 >> + auto& toEnumeration = static_cast<unsigned&>(*to->values[1]); > > Aren't these const too? Fixed. >> Source/WebCore/svg/SVGAnimatedColor.cpp:48 >> + auto& toColor = static_cast<Color&>(*to->values[0]); > > Yeah, I think we need a better way to convert from SVGValue to the underlying types. Fixed. >> Source/WebCore/svg/SVGAnimatedNumberOptionalNumber.cpp:79 >> + const auto& formSecond = m_animationElement->animationMode() == ToAnimation ? static_cast<float&>(*animated->values[1]) : static_cast<float&>(*from->values[1]); > > Typo: from Fixed. >> Source/WebCore/svg/SVGAnimatedNumberOptionalNumber.cpp:88 >> + m_animationElement->animateAdditiveNumber(percentage, repeatCount, formSecond, toSecond, toAtEndOfDurationSecond, animatedSecond); > > Typo: from Fixed. >> Source/WebCore/svg/SVGAnimatedType.h:57 >> + static std::unique_ptr<SVGAnimatedType> create(PropertyType&& property) > > I don't understand why these are public but the constructor is also public. Which should people use? Can the actual constructor be private? The constructors were private :). But Sam wanted to use std::make_unique, so I had to make them public. See https://bugs.webkit.org/show_bug.cgi?id=168586#c2. >> Source/WebCore/svg/SVGAnimatedType.h:107 >> + ASSERT(values[0] && !values[1]); > > Why is it an error to call toString on something with two values? The deleted functions SVGAnimatedType::valueAsString() and SVGAnimatedType::setValueAsString() were expecting only six single value SVGAnimatedType. And we were asserting that only these six types are the ones that we allow calling these two functions. With this patch, we are asserting here that this two functions will not be called for double value SVGAnimatedType. And SVGValue::toString() and SVGValue::parse() will assert if the SVGPropertyTraits does not implement toString or parse() functions. Besides we do not have a SVGPropertyTraits for a double valued SVGAnimatedType. >> Source/WebCore/svg/SVGAnimatedType.h:141 >> + [](const auto&) { return AnimatedUnknown; } > > I don't think we usually indent things this way. Fixed. >> Source/WebCore/svg/SVGAnimatedType.h:145 >> + RefPtr<SVGValue> values[2]; > > Rather than have an array, why not separate first and second members? > > RefPtr<SVGValue> first; > RefPtr<SVGValue> second; Fixed, >> Source/WebCore/svg/SVGFEConvolveMatrixElement.cpp:285 >> + divisorValue += kernelMatrix.propertyAt(i); > > Can you use a for : loop here? Fixed.

Created attachment 336505 [details] Patch

Created attachment 336508 [details] Patch

Created attachment 336521 [details] Patch

Created attachment 337942 [details] Patch

Comment on attachment 337942 [details] Patch Attachment 337942 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/7311818 New failing tests: animations/needs-layout.html

Created attachment 337952 [details] Archive of layout-test-results from ews101 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6

<rdar://problem/41333684>

The approach of 191237 is better than the one taken by the attached patch. *** This bug has been marked as a duplicate of bug 191237 ***