[webkit-reviews] review granted: [Bug 217623] CSSStyleDeclaration breaks JS spec (properties not showing up in Object.getOwnPropertyNames) : [Attachment 411481] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 217623: CSSStyleDeclaration breaks JS spec (properties not showing up in
Object.getOwnPropertyNames)
https://bugs.webkit.org/show_bug.cgi?id=217623

Attachment 411481: Patch

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




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

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

> Source/WebCore/DerivedSources.make:1422
> +    CSSStyleDeclaration+PropertyNames.idl \

Maybe we should use $(CSS_PROPERTY_NAME_FILES) here to make this rule a little
less long.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3967
> +    return $param if $param =~ /^CSSOM/;

Maybe we should switch to this approach for some of the other prefixes later?

> Source/WebCore/css/CSSStyleDeclaration.cpp:309
> +    String stringValue = WTF::switchOn(value, [&] (const auto& input) {

auto?

> Source/WebCore/css/CSSStyleDeclaration.h:75
> +    ExceptionOr<void> setPropertyValueInternal(CSSPropertyID, String);

How did you decide on String vs. const String& here?

> Source/WebCore/css/CSSStyleDeclaration.h:77
> +    ExceptionOr<void>
setPropertyValueInternalForPosOrPixelPrefixed(CSSPropertyID, Variant<double,
String>);

How did you decide on Variant vs. const Variant& here?

> LayoutTests/ChangeLog:13
> +	   Remove tests for iteration order, which is not standardized, and not
consistent among
> +	   browsers.

Makes me a little sad; since someone could rely on the order by accident,
wouldn’t there be some value in standardizing it, working towards being
consistent among browsers some day, and testing that it matches what we propose
for the standard?