Bug 174529 – [WebIDL] Replace some custom bindings code in JSCSSStyleDeclarationCustom.cpp with named getters/setters

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

[WebIDL] Replace some custom bindings code in JSCSSStyleDeclarationCustom.cpp with named getters/setters

[Sam Weinig]

Created attachment 315488 [details]
Patch

[Sam Weinig]

Created attachment 315490 [details]
Patch

[Sam Weinig]

Created attachment 315499 [details]
Patch

[Sam Weinig]

Created attachment 315505 [details]
Patch

[Build Bot]

Comment on attachment 315505 [details]
Patch

Attachment 315505 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4122497

New failing tests:
fast/text/international/block-flow-parser-test.html
imported/w3c/web-platform-tests/WebIDL/ecmascript-binding/put-forwards.html
fast/dom/StyleSheet/gc-rule-children-wrappers.html
fast/css/inherit-initial-shorthand-values.html
fast/dom/gc-9.html
fast/dom/CSSStyleDeclaration/cssstyledeclaration-properties-descriptor.html

[Build Bot]

Created attachment 315513 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 315505 [details]
Patch

Attachment 315505 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4122600

New failing tests:
fast/text/international/block-flow-parser-test.html
imported/w3c/web-platform-tests/WebIDL/ecmascript-binding/put-forwards.html
fast/dom/StyleSheet/gc-rule-children-wrappers.html
fast/css/inherit-initial-shorthand-values.html
fast/dom/gc-9.html
fast/dom/CSSStyleDeclaration/cssstyledeclaration-properties-descriptor.html

[Build Bot]

Created attachment 315516 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 315505 [details]
Patch

Attachment 315505 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4122637

New failing tests:
fast/text/international/block-flow-parser-test.html
imported/w3c/web-platform-tests/WebIDL/ecmascript-binding/put-forwards.html
fast/dom/StyleSheet/gc-rule-children-wrappers.html
fast/css/inherit-initial-shorthand-values.html
fast/dom/gc-9.html
fast/dom/CSSStyleDeclaration/cssstyledeclaration-properties-descriptor.html

[Build Bot]

Created attachment 315518 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 315505 [details]
Patch

Attachment 315505 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4122722

New failing tests:
fast/text/international/block-flow-parser-test.html
imported/w3c/web-platform-tests/WebIDL/ecmascript-binding/put-forwards.html
fast/dom/StyleSheet/gc-rule-children-wrappers.html
fast/css/inherit-initial-shorthand-values.html
fast/dom/gc-9.html
fast/dom/CSSStyleDeclaration/cssstyledeclaration-properties-descriptor.html

[Build Bot]

Created attachment 315521 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Sam Weinig]

Created attachment 315554 [details]
Patch

[Sam Weinig]

Created attachment 315555 [details]
Patch

[Build Bot]

Comment on attachment 315555 [details]
Patch

Attachment 315555 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4126488

New failing tests:
imported/w3c/web-platform-tests/WebIDL/ecmascript-binding/put-forwards.html

[Build Bot]

Created attachment 315558 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 315555 [details]
Patch

Attachment 315555 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4126498

New failing tests:
imported/w3c/web-platform-tests/WebIDL/ecmascript-binding/put-forwards.html

[Build Bot]

Created attachment 315559 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Build Bot]

Comment on attachment 315555 [details]
Patch

Attachment 315555 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4126559

New failing tests:
imported/w3c/web-platform-tests/WebIDL/ecmascript-binding/put-forwards.html

[Build Bot]

Created attachment 315561 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Sam Weinig]

Created attachment 315566 [details]
Patch

[Chris Dumez]

Comment on attachment 315566 [details]
Patch

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

> Source/WebCore/css/CSSStyleDeclaration.idl:47
> +    // these should be normal attributes, but there are too many combinations with

Where is this is the spec? I could not find those normal attributes at:
https://drafts.csswg.org/cssom/#the-cssstyledeclaration-interface

Is our behavior really standard? Do other browsers really behave the same way as us?

[Sam Weinig]

(In reply to Chris Dumez from comment #22)
> Comment on attachment 315566 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315566&action=review
> 
> > Source/WebCore/css/CSSStyleDeclaration.idl:47
> > +    // these should be normal attributes, but there are too many combinations with
> 
> Where is this is the spec? I could not find those normal attributes at:
> https://drafts.csswg.org/cssom/#the-cssstyledeclaration-interface
> 
> Is our behavior really standard? Do other browsers really behave the same
> way as us?

That spec has crazy formatting for me :(.

The property bits are defined in:

https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-camel-cased-attribute
https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-webkit-cased-attribute
https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-dashed-attribute

Note the text like:

"For each CSS property property that is a supported CSS property, the following partial interface applies where camel-cased attribute is obtained by running the CSS property to IDL attribute algorithm for property."

[Sam Weinig]

(In reply to Sam Weinig from comment #23)
> (In reply to Chris Dumez from comment #22)
> > Is our behavior really standard? Do other browsers really behave the same
> > way as us?

Not sure what the compat story is here. I tried to minimize behavior change as much as possible here. Matching other browsers and the spec can come next.

[Chris Dumez]

Comment on attachment 315566 [details]
Patch

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

r=me with comments.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:906
> +    push(@$outputArray, $indent . "bool propertySupported = true;\n") if $namedSetterOperation->extendedAttributes->{PutOnlyForSupportedProperties};

nit: Variable name should have "is" prefix.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:965
> +        if ($namedSetterOperation->extendedAttributes->{PutOnlyForSupportedProperties}) {

Isn't this confusingly named? What the code seems to do is call JSObject::put() if the property is NOT supported. The name seems to indicate the opposite.

> Source/WebCore/css/CSSStyleDeclaration.h:74
> +    // Bindings support

Missing period at the end.

> Source/WebCore/css/CSSStyleDeclaration.idl:49
> +    // recosider this decision.

Typo.

[Sam Weinig]

(In reply to Chris Dumez from comment #25)
> Comment on attachment 315566 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315566&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:965
> > +        if ($namedSetterOperation->extendedAttributes->{PutOnlyForSupportedProperties}) {
> 
> Isn't this confusingly named? What the code seems to do is call
> JSObject::put() if the property is NOT supported. The name seems to indicate
> the opposite.

Yeah, I guess. I was thinking of Put as meaning the custom setter, but that is not at all clear from the name. Renamed it to [CallNamedSetterOnlyForSupportedProperties].

[Sam Weinig]

Committed r219622: <http://trac.webkit.org/changeset/219622>