Bug 190303

Created attachment 351657 [details] Patch

I moved the serialization code because this change uncovered a bug in the way I was parsing initial values.

Comment on attachment 351657 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351657&action=review Looks generally fine, but one more round would be good. > Source/WebCore/css/CSSCalculationValue.cpp:282 > + void getDirectComputationalDependencies(HashSet<CSSPropertyID>& values) const final > + { > + auto dependencies = m_value->getDirectComputationalDependencies(); > + values.add(dependencies.begin(), dependencies.end()); > + } > + > + void getDirectRootComputationalDependencies(HashSet<CSSPropertyID>& values) const final > + { > + auto dependencies = m_value->getDirectRootComputationalDependencies(); > + values.add(dependencies.begin(), dependencies.end()); > + } Maybe all versions of these functions should take the HashSet as an argument? It would avoid moving values from one hash to another. > Source/WebCore/css/CSSCalculationValue.h:73 > + virtual void getDirectComputationalDependencies(HashSet<CSSPropertyID>&) const = 0; > + virtual void getDirectRootComputationalDependencies(HashSet<CSSPropertyID>&) const = 0; collect* instead of get* would be better names for these. > Source/WebCore/css/CSSCalculationValue.h:108 > + HashSet<CSSPropertyID> getDirectComputationalDependencies() const; > + HashSet<CSSPropertyID> getDirectRootComputationalDependencies() const; Here too. It signals they are not cheap accessors. > Source/WebCore/css/CSSCustomPropertyValue.cpp:51 > +Vector<CSSParserToken>& CSSCustomPropertyValue::tokens(const CSSRegisteredCustomPropertySet& registeredProperties, const RenderStyle& style) const > +{ > + m_tokensReturnValue.clear(); It appears m_tokensReturnValue is only needed so you can return a reference? It is not clear to me how that is better than just returning the vector as a value and it is certainly more bug prone. Move semantics should keep it cheap. An alternative is the pass the vector in as a reference parameter (and call this something like collectTokens). > Source/WebCore/css/CSSPrimitiveValue.cpp:1231 > + return HashSet<CSSPropertyID>({CSSPropertyFontSize}); I think you can just say return { CSSPropertyFontSize }; > Source/WebCore/css/CSSPrimitiveValue.cpp:1235 > + return HashSet<CSSPropertyID>(); return { }; > Source/WebCore/css/CSSPrimitiveValue.cpp:1246 > + return HashSet<CSSPropertyID>(); return { }; > Source/WebCore/css/CSSValue.cpp:124 > + return HashSet<CSSPropertyID>(); return { }; > Source/WebCore/css/CSSValue.cpp:131 > + return HashSet<CSSPropertyID>(); return { };

Created attachment 351682 [details] Patch

Comment on attachment 351682 [details] Patch Clearing flags on attachment: 351682 Committed r236895: <https://trac.webkit.org/changeset/236895>

All reviewed patches have been landed. Closing bug.

<rdar://problem/45067859>