[webkit-reviews] review granted: [Bug 217802] Unify CSS property enablement (-apple-color-filter should not exist on CSSStyleDeclaration by default) : [Attachment 460619] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Sam Sneddon [:gsnedders]
<gsnedders at apple.com>'s request for review:
Bug 217802: Unify CSS property enablement (-apple-color-filter should not exist
on CSSStyleDeclaration by default)
https://bugs.webkit.org/show_bug.cgi?id=217802

Attachment 460619: Patch

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




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

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

nice!

> Source/WebCore/css/CSSComputedStyleDeclaration.h:144
> +    const FixedVector<CSSPropertyID>& getActiveComputedPropertyIDs() const;

WebKit coding style does not use get* for getters (examples to the contrary are
DOM API implementations or old leftovers). "Exposed" could  be a better name
for this concept than "active computed", you use it elsewhere and in the prose
too. So exposedCSSPropertyIDs() perhaps?

> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:355
> +bool PropertySetCSSStyleDeclaration::isCSSPropertyExposed(CSSPropertyID
propertyID) const

or isExposedCSSProperty, similar to existing
isInternalCSSProperty/isEnabledCSSProperty etc.

> Source/WebCore/dom/Document.cpp:648
> +    {
> +	   std::array<CSSPropertyID, numComputedPropertyIDs> exposed;
> +	   auto last = std::copy_if(std::begin(computedPropertyIDs),
std::end(computedPropertyIDs),
> +	       exposed.begin(), [&](CSSPropertyID x) {
> +		   return isCSSPropertyExposed(x, m_settings.ptr());
> +	       });
> +
> +	   FixedVector<CSSPropertyID> active(std::distance(exposed.begin(),
last));
> +	   std::copy(exposed.begin(), last, active.begin());
> +	   m_activeComputedPropertyIDs = active;
> +    }

This work is not needed unless the page actually uses computed style APIs.
Initialization could by done lazily in getActiveComputedPropertyIDs (though it
is not that expensive either).

> Source/WebCore/dom/Document.h:1604
> +    const FixedVector<CSSPropertyID>& getActiveComputedPropertyIDs() const {
return m_activeComputedPropertyIDs; }

Similarly exposedCSSPropertyIDs (including "CSS" since the document scope
doesn't imply it).