Bug 190274 – DOMCSSNamespace (window.CSS) should be a

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Layout and Rendering

[Justin Michaud]

Make all the CSS.* methods instance methods. This matches a recent change to <https://drafts.csswg.org/cssom-1/#namespacedef-css>, although no other browsers implement this yet. This will be required to make the CSS.paintWorklet attribute work.

[Justin Michaud]

Created attachment 351580 [details]
Patch

[EWS Watchlist]

Comment on attachment 351580 [details]
Patch

Attachment 351580 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9450533

New failing tests:
fast/css/css-prototype.html

[EWS Watchlist]

Created attachment 351584 [details]
Archive of layout-test-results from ews102 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 351580 [details]
Patch

Attachment 351580 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9450555

New failing tests:
fast/css/css-prototype.html

[EWS Watchlist]

Created attachment 351585 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

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

[Ryosuke Niwa]

Comment on attachment 351580 [details]
Patch

Looks like the test is failing. The patch looks good otherwise. r- for the failing tests.

[Chris Dumez]

Comment on attachment 351580 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        DOMCSSNamespace (window.CSS) should be a

Title seems wrong.

> Source/WebCore/ChangeLog:9
> +        This matches <https://drafts.csswg.org/cssom-1/#namespacedef-css>, although no other browsers

To be clear, the spec indicate a namespace should be used, which is not what you did. Our bindings generator does not support namespaces yet AFAIK and your implementation does not seem to mimic a namespace either.

> Source/WebCore/css/DOMCSSNamespace.idl:34
> +    NoInterfaceObject

Exposed=Window and NoInterfaceObject at the same time make no sense. I would suggest dropping Exposed=Window.

> Source/WebCore/page/DOMWindow.cpp:1576
> +Ref<DOMCSSNamespace> DOMWindow::css() const

We do not normally return Ref<> / RefPtr<> expect for factories (where ownership is transferred). This should return a DOMCSSNamespace&.

> Source/WebCore/page/DOMWindow.idl:147
> +    [Replaceable] readonly attribute DOMCSSNamespace CSS;

Please add testing to confirm that the CSS property has the following attributes (as per Web IDL spec):
{ [[Writable]]: true, [[Enumerable]]: false, [[Configurable]]: true }

Also, CSS.__proto__ should be Object.prototype as per Web IDL. I doubt this is correct with your implementation.

> LayoutTests/fast/css/css-prototype-expected.txt:7
> +PASS Object.getOwnPropertyNames(CSS.__proto__).includes('supports') is true

I might be misinterpreting the Web IDL specs but it looks to me like the properties should be on the instance, not the prototype. As a matter of fact, the prototype is supposed to be the regular ObjectPrototype.

[Chris Dumez]

Comment on attachment 351580 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        This matches <https://drafts.csswg.org/cssom-1/#namespacedef-css>, although no other browsers
> 
> To be clear, the spec indicate a namespace should be used, which is not what you did. Our bindings generator does not support namespaces yet AFAIK and your implementation does not seem to mimic a namespace either.

Relevant spec:
- https://heycam.github.io/webidl/#es-namespaces

[Simon Fraser (smfr)]

Should be a what?