[webkit-reviews] review granted: [Bug 217802] -apple-color-filter should not exist on CSSStyleDeclaration by default : [Attachment 427807] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Sneddon [:gsnedders]
<gsnedders at apple.com>'s request for review:
Bug 217802: -apple-color-filter should not exist on CSSStyleDeclaration by
default
https://bugs.webkit.org/show_bug.cgi?id=217802

Attachment 427807: Patch

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




--- Comment #14 from Darin Adler <darin at apple.com> ---
Comment on attachment 427807
  --> https://bugs.webkit.org/attachment.cgi?id=427807
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1472
> +	   CSSPropertyID propertyID = computedPropertyIDs[i];

I prefer auto here.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1475
> +	   auto name = getPropertyName(propertyID);

Not sure we need to stick this into a local; OK, I guess, but could also just
call it on the append line like we did before.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1478
> +	       // FIXME: ideally we'd assert in this case, but currently it's
hit too often

WebKit coding style says write this like a sentence with a capitalized first
word and a full stop at the end (which I call a period because educated in
U.S.).

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3997
> +	   /* Internal properties should be handled by isCSSPropertyExposed
above */

WebKit coding style encourages use of C++ style // comments, even if code
nearby was done in a different style (not sure why it was, maybe imported from
Chromium).

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4223
> +	   auto propertyID = set[i];

While this probably how I would write it, there are others who discourage me
from caching simply computed values like this in local variables.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4225
> +	       list.append(CSSProperty(propertyID, WTFMove(value), false));

After doing reserveInitialCapacity, can and should use uncheckedAppend instead
of append for a little tighter code. Not sure why the old code was not doing
that.

> Source/WebCore/css/CSSStyleDeclaration.cpp:203
> -    if (!isEnabledCSSProperty(id) || !isCSSPropertyEnabledBySettings(id,
settings))
> +    if (!isCSSPropertyExposed(id, settings))

So presumably this is a behavior change, and for the better, by handling more
cases. We should spend a moment and reflect on how we could detect that with
tests.

> Source/WebCore/css/DOMCSSNamespace.cpp:64
> -    if (parserContext.isPropertyRuntimeDisabled(propertyID))
> +    if (!isCSSPropertyExposed(propertyID, &document.settings()))

Ditto.

> Source/WebCore/css/StyleProperties.cpp:863
> +    if (!isCSSPropertyExposed(propertyID, &parserContext.propertySettings)
&& !isInternalCSSProperty(propertyID)) {

I sort of expected a single helper function call here, not two. What makes this
different enough from the other call sites that it needs an explicit
isInternalCSSProperty check and they do not?

> Source/WebCore/css/makeprop.pl:337
> +static bool isEnabledCSSProperty(const CSSPropertyID);
> +static bool isCSSPropertyEnabledBySettings(const CSSPropertyID, const
Settings*);
> +static bool isCSSPropertyEnabledByCSSPropertySettings(const CSSPropertyID,
const CSSPropertySettings*);

These should all say "CSSPropertyID", not "const CSSPropertyID".

But also, why do we need to declare them at the top of the file? Why not just
define them?

> Source/WebCore/css/makeprop.pl:404
> +static bool isEnabledCSSProperty(const CSSPropertyID id)

Ditto.

> Source/WebCore/css/makeprop.pl:419
> +static bool isEnabledCSSProperty(const CSSPropertyID)

Ditto.

> Source/WebCore/css/makeprop.pl:428
> +static bool isCSSPropertyEnabledBySettings(const CSSPropertyID id, const
Settings* settings)

Ditto.

> Source/WebCore/css/makeprop.pl:448
> +static bool isCSSPropertyEnabledByCSSPropertySettings(const CSSPropertyID
id, const CSSPropertySettings* settings)

Ditto.

> Source/WebCore/css/makeprop.pl:465
> +    return true;

This isn’t needed, because we have the default above.

> Source/WebCore/css/makeprop.pl:468
> +bool isCSSPropertyExposed(const CSSPropertyID id, const Settings* settings)

Ditto, no const CSSPropertyID.

> Source/WebCore/css/makeprop.pl:473
> +bool isCSSPropertyExposed(const CSSPropertyID id, const CSSPropertySettings*
settings)

Ditto.

> Source/WebCore/css/makeprop.pl:732
>  bool isInternalCSSProperty(const CSSPropertyID);
> -bool isEnabledCSSProperty(const CSSPropertyID);
> -bool isCSSPropertyEnabledBySettings(const CSSPropertyID, const Settings* =
nullptr);
> +bool isCSSPropertyExposed(const CSSPropertyID, const Settings*);
> +bool isCSSPropertyExposed(const CSSPropertyID, const CSSPropertySettings*);

Same here, just CSSPropertyID, no const.

> Source/WebCore/css/makeprop.pl:735
>  const WTF::AtomString& getPropertyNameAtomString(CSSPropertyID id);
>  WTF::String getPropertyNameString(CSSPropertyID id);

Should take these "id" out of here.

> Source/WebCore/css/parser/CSSParserContext.cpp:87
> +    , propertySettings { CSSPropertySettings(document.settings()) }

Could use braces instead of parentheses here if you like.

    , propertySettings { CSSPropertySettings { document.settings() } }

> Source/WebCore/css/parser/CSSParserImpl.cpp:813
> -    if (m_context.isPropertyRuntimeDisabled(propertyID))
> +    if (!isCSSPropertyExposed(propertyID, &m_context.propertySettings))

So presumably this is a behavior change, and for the better, by handling more
cases. We should spend a moment and reflect on how we could detect that with
tests.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:114
>	   auto propertyID = static_cast<CSSPropertyID>(hashTableEntry->id);
> -	   // FIXME: Should take account for flags in settings().
> -	   if (isEnabledCSSProperty(propertyID))
> -	       return propertyID;
> +	   return propertyID;

I do not think we need a local variable here or braces.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:204
> -    if (!isEnabledCSSProperty(property))
> +    if (!isCSSPropertyExposed(property, &m_context.propertySettings) &&
!isInternalCSSProperty(property)) {

So presumably this is a behavior change, and for the better, by handling more
cases. We should spend a moment and reflect on how we could detect that with
tests.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:278
> +    if (!isCSSPropertyExposed(property, &context.propertySettings)) {
> +	   ASSERT_NOT_REACHED();
> +	   return nullptr;
> +    }

Is it important to add the check here? All these new places we are checking ...
are they detectable? Is the benefit substantial enough to justify additional
runtime overhead.

Maybe just an assertion instead of a always-compiled check to back the
assertion. I’m concerned about adding all these additional checks just to be
assertions.