[webkit-reviews] review denied: [Bug 230337] Parsing support for font-palette-values : [Attachment 438756] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 230337: Parsing support for font-palette-values
https://bugs.webkit.org/show_bug.cgi?id=230337

Attachment 438756: Patch

https://bugs.webkit.org/attachment.cgi?id=438756&action=review




--- Comment #15 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 438756
  --> https://bugs.webkit.org/attachment.cgi?id=438756
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=438756&action=review

> Source/WebCore/css/CSSFontPaletteValuesRule.cpp:75
> +
> +void CSSFontPaletteValuesRule::setFontFamily(String&& fontFamily)
> +{
> +    m_fontPaletteValuesRule->setFontFamily(fontFamily);
> +}

This needs CSSStyleSheet::RuleMutationScope. See CSSKeyframesRule::setName for
example.

If it is important to have a mutable CSSOM API then it needs to be tested also
in shared stylesheet case (in WPT too). For example, use the same exact
stylesheet in two different shadow roots, mutate one and check the other one
doesn't mutate too.

> Source/WebCore/css/CSSFontPaletteValuesRule.cpp:77
> +void CSSFontPaletteValuesRule::setBasePalette(String&& basePalette)

Same thing here.

> Source/WebCore/css/CSSFontPaletteValuesRule.cpp:99
> +void CSSFontPaletteValuesRule::setFromMapLike(unsigned key, String&& value)

And here.

> Source/WebCore/css/CSSFontPaletteValuesRule.idl:27
> +    maplike<unsigned long, CSSOMString>;
> +    attribute CSSOMString fontFamily;
> +    attribute CSSOMString basePalette;

Why do people keep adding mutable things to CSSOM? What is this stuff good for?