[webkit-reviews] review denied: [Bug 189694] Implement CSS Custom Properties and Values Skeleton : [Attachment 350009] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 189694: Implement CSS Custom Properties and Values Skeleton
https://bugs.webkit.org/show_bug.cgi?id=189694

Attachment 350009: Patch

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




--- Comment #6 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 350009
  --> https://bugs.webkit.org/attachment.cgi?id=350009
Patch

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

> Source/WebCore/ChangeLog:19
> +	   * bindings/scripts/CodeGeneratorJS.pm:

Please add bindings tests for this new functionality.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3100
> +	   #next if ($operation->isStatic);

Please don't check in commented out code.

> Source/WebCore/css/DOMCSSNamespace.idl:37
> +    [CallWith=Document,EnabledAtRuntime=CSSCustomPropertiesAndValues] static
void registerProperty(DOMCSSPropertyDescriptor descriptor);

This should probably be in it's own IDL file using a partial interface.

> Source/WebCore/dom/Document.cpp:8224
> +    if (m_CSSRegisteredPropertySet.contains(prop.name))
> +	   return false;
> +    m_CSSRegisteredPropertySet.add(prop.name, prop);
> +    return true;

This should use HashMap::ensure to avoid the double hash lookup. Also, is the
return value necessary? It doesn't seem like the caller uses it.

> Source/WebCore/dom/Document.h:1496
> +    bool registerCSSProperty(const CSSRegisteredProperty&);

This could probably take an r-value CSSRegisteredProperty.

> Source/WebKit/Shared/WebPreferences.yaml:1358
> +  humanReadableName: "CSS Custom Properties and Values Api Level 1"
> +  humanReadableDescription: "Enable CSS Properties and Values Api Level 1"

"Api" -> "API". Also, the "Level" 1 is probably not necessary.