Bug 190038

Created attachment 351040 [details] Patch

Created attachment 351045 [details] Patch

Comment on attachment 351045 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351045&action=review You might want to wait for smfr to take a look at this. Otherwise, I like it! > Source/WebCore/ChangeLog:26 > + * css/CSSComputedStyleDeclaration.cpp: > + (WebCore::ComputedStyleExtractor::customPropertyValue): Since this is a pretty large patch, can you provide short summaries of what you changed in this section? You don't need to do it for every line, or even every file, but it's nice to split it into groups of related code and say what happened. e.g. Changed "CustomPropertyValueMap" to "CustomPropertyValueCascade". > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2631 > + // TODO this should be done based on the syntax Nit: End comment with . > Source/WebCore/css/CSSCustomPropertyValue.h:82 > + Length* resolvedTypedValue() { return m_resolvedTypedValue.get(); } > + const Length* resolvedTypedValue() const { return m_resolvedTypedValue.get(); } > + void setResolvedTypedValue(Length length) { m_resolvedTypedValue = std::make_unique<Length>(WTFMove(length)); } Do you think this might be better named resolvedLengthValue()? At the moment you're only resolving Lengths - will that always be the case? > Source/WebCore/css/CSSCustomPropertyValue.h:119 > + // TODO It should not be possible to express an invalid state, such as containsVariables() && resolvedTypedValue() Nit: Say "FIXME:" instead of "TODO". End comment with . > Source/WebCore/css/CSSValue.cpp:493 > + auto* val = map? map->get(name) : nullptr; Nit: Space before ? I'm trying to see if there is a better way to write this.... for (auto* map : m_map) { if (!map) continue; if (auto* val = map->get(name)) return val; } I'm not sure it is better. > Source/WebCore/css/CSSValue.h:280 > +// Wrapper class to merge a number of CustomPropertyValueMaps into one, picking the first map > +// that contains a particular key Nit: End comment with . > Source/WebCore/css/CSSValue.h:284 > + CustomPropertyValueMapCascade(const Vector<const CustomPropertyValueMap*>& map) > + : m_map { map } { } Style Nit: CustomPropertyValueMapCascade(const Vector<const CustomPropertyValueMap*>& map) : m_map { map } { } > Source/WebCore/css/CSSVariableData.cpp:147 > + // TODO we should not have to serialize a value that was already resolved Nit: FIXME: and . > Source/WebCore/css/StyleBuilderCustom.h:1880 > +// These are called directly from the generated code, not based off the json file I don't think you need this comment. > Source/WebCore/css/StyleBuilderCustom.h:1883 > + auto* initialVal = registered? registered->initialValue.get() : nullptr; Nit: Space before ? > Source/WebCore/css/StyleResolver.cpp:2279 > + // TODO cycles should be detected Nit: FIXME: and . > Source/WebCore/css/parser/CSSPropertyParser.cpp:3953 > + case CSSPropertyCustom: // TODO this should be based on syntax Nit: FIXME: and . > Source/WebCore/rendering/style/RenderStyle.cpp:2252 > + for (auto* customPropertyData : {&inheritedPropertyData, &nonInheritedPropertyData}) { Nit: Spaces around { and } > Source/WebCore/rendering/style/RenderStyle.cpp:2266 > + // Now insert invalid values, or defaults if the property is registered Nit: . > Source/WebCore/rendering/style/RenderStyle.cpp:2269 > + auto* registered = registeredProperties.get(property); This can go down where it is first used.

Created attachment 351107 [details] Patch

Created attachment 351124 [details] Patch

Comment on attachment 351124 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351124&action=review > Source/WebCore/css/StyleResolver.cpp:1639 > + CSSRegisteredCustomProperty* customPropertyRegistered; This is ub, which is why the windows build failed I think. Why did nothing else catch this?

Comment on attachment 351124 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351124&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2628 > + auto* value = CustomPropertyValueMapCascade({ &style->inheritedCustomProperties(), &style->nonInheritedCustomProperties() }).get(propertyName); That's a pretty indirect way of doing two hash lookups. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4166 > - auto results = copyToVector(customProperties.keys()); > - return results.at(index); > + const auto& inheritedCustomProperties = style->inheritedCustomProperties(); > + > + if (i < numComputedProperties + inheritedCustomProperties.size()) { > + auto results = copyToVector(inheritedCustomProperties.keys()); > + return results.at(i - numComputedProperties); > + } > + > + const auto& nonInheritedCustomProperties = style->nonInheritedCustomProperties(); > + auto results = copyToVector(nonInheritedCustomProperties.keys()); > + return results.at(i - inheritedCustomProperties.size() - numComputedProperties); This code is weird. We are indexing into hash maps that have completely random order. There is also unnecessary copying into a vector. I know this patch didn't add these things but still... > Source/WebCore/css/CSSCustomPropertyValue.h:81 > + Length* resolvedTypedValue() { return m_resolvedTypedValue.get(); } > + const Length* resolvedTypedValue() const { return m_resolvedTypedValue.get(); } Returning std::optional<Length> would be more modern. > Source/WebCore/css/CSSCustomPropertyValue.h:120 > + // FIXME: It should not be possible to express an invalid state, such as containsVariables() && resolvedTypedValue(). > + std::unique_ptr<Length> m_resolvedTypedValue { nullptr }; I think you want std::optional. Also explicit initialization is unnecessary. > Source/WebCore/css/CSSValue.cpp:499 > +CSSCustomPropertyValue* CustomPropertyValueMapCascade::get(const AtomicString& name) const > +{ > + for (auto* map : m_map) { > + if (!map) > + continue; > + if (auto* val = map->get(name)) > + return val; > + } > + return nullptr; > +} Maybe this should just be a helper on RenderStyle? (see below) > Source/WebCore/css/CSSValue.h:292 > +// Wrapper class to merge a number of CustomPropertyValueMaps into one, picking the first map > +// that contains a particular key. > +class CustomPropertyValueMapCascade { > +public: > + CustomPropertyValueMapCascade(const Vector<const CustomPropertyValueMap*>& map) > + : m_map { map } > + { > + } > + > + CSSCustomPropertyValue* get(const AtomicString& name) const; > + > +private: > + const Vector<const CustomPropertyValueMap*> m_map; > +}; What guarantees the CustomPropertyValueMaps stay alive during lifetime of this object? > Source/WebCore/css/StyleBuilderCustom.h:1883 > + auto* initialVal = registered ? registered->initialValue.get() : nullptr; > + if (initialVal) { initialVal local doesn't add anything and could be removed. Also WebKit style is to use full words ('initialValue'). > Source/WebCore/css/StyleBuilderCustom.h:1903 > +inline void StyleBuilderCustom::applyValueCustomProperty(StyleResolver& styleResolver, const CSSRegisteredCustomProperty* registered, CSSValue& value) > +{ > + auto* customPropertyValue = &downcast<CSSCustomPropertyValue>(value); How do we know this cast is safe? The function should probably be taking CSSCustomPropertyValue and leaving casting to the clients. > Source/WebCore/css/StyleResolver.cpp:1710 > + CustomPropertyValueMapCascade customProperties({ &m_state.style()->inheritedCustomProperties(), &m_state.style()->nonInheritedCustomProperties() }); > + > + return parser.parseValueWithVariableReferences(propID, value, customProperties, document().getCSSRegisteredCustomPropertySet(), *m_state.style()); You now pass both CustomPropertyValueMapCascade and style almost everywhere. This makes the whole CustomPropertyValueMapCascade type sort of pointless (and it doesn't do much in the first place) as all the same information is available from RenderStyle. How about just getting rid of CustomPropertyValueMapCascade and simply adding a property name getter to RenderStyle that resolves the value from right maps? > Source/WebCore/css/makeprop.pl:1041 > + if (isInitial) > + StyleBuilderCustom::applyInitialCustomProperty(styleResolver, registered, downcast<CSSCustomPropertyValue>(value).name()); > + else if (isInherit) > + StyleBuilderCustom::applyInheritCustomProperty(styleResolver, registered, downcast<CSSCustomPropertyValue>(value).name()); > + else > + StyleBuilderCustom::applyValueCustomProperty(styleResolver, registered, value); Why does't call to applyValueCustomProperty cast the value to CSSCustomPropertyValue here like the other functions do? > Source/WebCore/css/parser/CSSParser.cpp:216 > + if (value.isVariableReferenceValue()) { > + const CSSVariableReferenceValue& valueWithReferences = downcast<CSSVariableReferenceValue>(value); is<CSSVariableReferenceValue>(value) pairs better with downcast<> > Source/WebCore/css/parser/CSSPropertyParser.cpp:3953 > + case CSSPropertyCustom: // FIXME this should be based on syntax. I don't understand the comment. > Source/WebCore/rendering/style/RenderStyle.cpp:2249 > - if (!m_rareInheritedData->customProperties->containsVariables) > - return; > + auto& inheritedPropertyData = m_rareInheritedData.access().customProperties.access(); > + auto& nonInheritedPropertyData = m_rareNonInheritedData.access().customProperties.access(); This now copies both rareInheritedData and rareNonInheritedData unconditionally. You should check what actually needs to be mutated before triggering COW.

Comment on attachment 351124 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351124&action=review >> Source/WebCore/css/parser/CSSParser.cpp:216 >> + const CSSVariableReferenceValue& valueWithReferences = downcast<CSSVariableReferenceValue>(value); > > is<CSSVariableReferenceValue>(value) pairs better with downcast<> Why does the original code use isVariableReferenceValue() instead of is<>()? Is it just legacy?

Created attachment 351341 [details] Patch

Created attachment 351342 [details] Patch

Created attachment 351415 [details] Patch

Comment on attachment 351415 [details] Patch Looks good, r=me

Comment on attachment 351415 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351415&action=review > Source/WebCore/css/CSSCustomPropertyValue.h:85 > + const std::optional<Length> resolvedTypedValue() const { return m_resolvedTypedValue; } This could return a reference. > Source/WebCore/rendering/style/RenderStyle.h:2006 > + for (auto* map : { &nonInheritedCustomProperties(), &inheritedCustomProperties() }) { Would simply for (auto& map : { nonInheritedCustomProperties(), inheritedCustomProperties() }) work (or would it end up creating copies of the maps)?

Comment on attachment 351415 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351415&action=review >> Source/WebCore/rendering/style/RenderStyle.h:2006 >> + for (auto* map : { &nonInheritedCustomProperties(), &inheritedCustomProperties() }) { > > Would simply > > for (auto& map : { nonInheritedCustomProperties(), inheritedCustomProperties() }) > > work (or would it end up creating copies of the maps)? I think we need some kind of wrapper on it, since you can't have an initialization list of references as far as I know.

Created attachment 351532 [details] Patch

Comment on attachment 351532 [details] Patch Clearing flags on attachment: 351532 Committed r236828: <https://trac.webkit.org/changeset/236828>

All reviewed patches have been landed. Closing bug.

<rdar://problem/44999769>