Bug 190237 – Implement feature flag and bindings for CSS Painting API

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Justin Michaud]

Implement a feature flag for CSS Painting API

[Justin Michaud]

Created attachment 351482 [details]
Patch

[EWS Watchlist]

Attachment 351482 [details] did not pass style-queue:


ERROR: Source/WebCore/features.json:0:  The feature "Pointer Events" has a specification field but no specification of that name exists.  [json/syntax] [5]
Total errors found: 1 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 351482 [details]
Patch

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

New failing tests:
css-custom-paint/basic.html

[EWS Watchlist]

Created attachment 351485 [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 351482 [details]
Patch

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

New failing tests:
css-custom-paint/basic.html

[EWS Watchlist]

Created attachment 351491 [details]
Archive of layout-test-results from ews115 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 351482 [details]
Patch

Attachment 351482 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9441837

New failing tests:
css-custom-paint/basic.html

[EWS Watchlist]

Created attachment 351521 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Justin Michaud]

Created attachment 351530 [details]
Patch

[EWS Watchlist]

Attachment 351530 [details] did not pass style-queue:


ERROR: Source/WebCore/features.json:0:  The feature "Pointer Events" has a specification field but no specification of that name exists.  [json/syntax] [5]
Total errors found: 1 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 351530 [details]
Patch

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

Can we split the patch to change the semantics of CSS into a separate patch?
This is a pretty massive patch, and we'd like to split each logical piece into its own patch if that's possible.

> LayoutTests/ChangeLog:12
> +        * css-custom-paint/basic-expected.txt: Added.
> +        * css-custom-paint/basic.html: Added.

This should probably live inside LayoutTests/fast instead.

> LayoutTests/ChangeLog:14
> +        * css3/css-prototype-expected.txt: Added.
> +        * css3/css-prototype.html: Added.

css3 is a directory into which we import tests. I know some folks have introduced new tests there but we should avoid that.
Again, these tests are best suited for fast/css.

[Justin Michaud]

Created attachment 351648 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 351648 [details]
Patch

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

> Source/WebCore/bindings/js/JSCSSPaintWorkletGlobalScopeCustom.cpp:35
> +namespace {

Just forget about anonymous namespace. Just add a static function.

> Source/WebCore/bindings/js/JSCSSPaintWorkletGlobalScopeCustom.cpp:36
> +using namespace WebCore;

I think this might be the culprit of the build failure.

> Source/WebCore/bindings/js/JSCSSPaintWorkletGlobalScopeCustom.cpp:66
> +    // STEP 1.

There's no need to say STEP x. That's just a pure noise to the code.

> LayoutTests/fast/css-custom-paint/basic.html:8
> +    paint(ctx, geom, properties) {

Please don't abbreviate context as ctx or geometry as geom.
I know it's a common practice but we try to spell out each word in WebKit.

> LayoutTests/fast/css-custom-paint/registerPaintBindings-expected.txt:1
> +CONSOLE MESSAGE: line 7: hello

What's up with this console log?

> LayoutTests/fast/css-custom-paint/registerPaintBindings.html:17
> +    var calls = [];

Use const & let instead throughout.

> LayoutTests/fast/css-custom-paint/registerPaintBindings.html:35
> +}, 'must rethrow an exception thrown while getting "prototype" property of the constructor');

Doesn't this test throw when we're running [[Get]] for inputProperties??

> LayoutTests/fast/css-custom-paint/registerPaintBindings.html:105
> +  var proxy = new Proxy(class extends MyPaint { }, {
> +      get: function (target, name) {

This doesn't really need a proxy. You just need to do:
class extends MyPaint { }
MyPaint.inputArguments = 1.

[Justin Michaud]

Comment on attachment 351648 [details]
Patch

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

>> LayoutTests/fast/css-custom-paint/registerPaintBindings.html:105
>> +      get: function (target, name) {
> 
> This doesn't really need a proxy. You just need to do:
> class extends MyPaint { }
> MyPaint.inputArguments = 1.

I wanted to check "calls"

[Justin Michaud]

Created attachment 351651 [details]
Patch

[EWS Watchlist]

Comment on attachment 351651 [details]
Patch

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

New failing tests:
fast/css-custom-paint/basic.html
fast/css-custom-paint/registerPaintBindings.html

[EWS Watchlist]

Created attachment 351654 [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

[Justin Michaud]

Created attachment 351655 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 351655 [details]
Patch

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

> Source/WebCore/bindings/js/JSCSSPaintWorkletGlobalScopeCustom.cpp:2
> + * Copyright (C) 2015-2017 Apple Inc. All rights reserved.

Nit: -2018

> LayoutTests/fast/css-custom-paint/basic.html:2
> +<!DOCTYPE html><!-- webkit-test-runner [ experimental:CSSCustomPaintEnabled=true ] -->
> +<script src="../../resources/testharness.js"></script>

Please put the author meta data as well as assertion description.
e.g. https://trac.webkit.org/browser/webkit/trunk/LayoutTests/fast/custom-elements/DOMImplementation-createDocument.html

> LayoutTests/fast/css-custom-paint/basic.html:10
> +test(function() {
> +  CSS.paintWorkletGlobalScope.registerPaint('test1', class {
> +    static get inputProperties() { return ["--test"]; }
> +    paint() { }
> +  });
> +}, 'test that registerPaint runs');

What is the assertion here? If we're expecting that the paint function to run, then you'd have to add that assertion.
Doest his test pass in Chrome? We should make sure whatever test we're adding passes in another implementation if there is one.
One way to do that might be to declare a local variable like `let didCallPaint = false` and set it to true in paint function.
Then, you can wait for requestAnimationFrame to happen and check that the flag is set.
You can wrap the whole thing in a Promise and use promise_test instead for that.

> LayoutTests/fast/css-custom-paint/registerPaintBindings.html:2
> +<!DOCTYPE html><!-- webkit-test-runner [ experimental:CSSCustomPaintEnabled=true ] -->
> +<script src="../../resources/testharness.js"></script>

Ditto about adding author & assertion description.

> LayoutTests/fast/css-custom-paint/registerPaintBindings.html:16
> +    let calls = [];
> +    let proxy = new Proxy(class extends MyPaint { }, {

Use const here and elsewhere in the test where possible.
`const` in JS doesn't mean the thing referenced doesn't change.
It means that the reference itself doesn't change.
e.g. it's like char* const as opposed to const char* in C.

> LayoutTests/fast/css-custom-paint/registerPaintBindings.html:52
> +test(function () {

Very nice test!

> LayoutTests/fast/css-custom-paint/registerPaintBindings.html:104
> +}, 'must rethrow an exception thrown while getting callbacks on the constructor prototype');

Nit: should read "while getting paint callback"

> LayoutTests/fast/css-custom-paint/registerPaintBindings.html:119
> +}, 'must rethrow an exception thrown while converting a callback value to Function callback type');

"while converting paint callback"?

> LayoutTests/fast/css-custom-paint/registerPaintBindings.html:135
> +test(function () {

Nice test!

[Justin Michaud]

Comment on attachment 351655 [details]
Patch

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

>> LayoutTests/fast/css-custom-paint/basic.html:10
>> +}, 'test that registerPaint runs');
> 
> What is the assertion here? If we're expecting that the paint function to run, then you'd have to add that assertion.
> Doest his test pass in Chrome? We should make sure whatever test we're adding passes in another implementation if there is one.
> One way to do that might be to declare a local variable like `let didCallPaint = false` and set it to true in paint function.
> Then, you can wait for requestAnimationFrame to happen and check that the flag is set.
> You can wrap the whole thing in a Promise and use promise_test instead for that.

This is just to test that registerPaint() itself does not throw. None of these tests pass in chrome, because chrome would require these tests to be run inside the paint worklet. Since there is no way to manipulate the DOM, I suspect that the standard test harness won't work.

[Justin Michaud]

Looking at https://github.com/web-platform-tests/wpt/tree/master/css/css-paint-api it seems they used the Paint API itself to do the testing, drawing different colors for pass/fail. Once the api actually works, we can revise these tests to use the real API and have the tests run in Chrome as well.

[Justin Michaud]

Created attachment 352367 [details]
Patch

[Justin Michaud]

Created attachment 352374 [details]
Patch

[Justin Michaud]

Created attachment 352381 [details]
Patch

[Justin Michaud]

Created attachment 352398 [details]
Patch

[Justin Michaud]

Created attachment 352405 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 352405 [details]
Patch

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

> Source/WebCore/Configurations/FeatureDefines.xcconfig:89
> +ENABLE_CSS_PAINT_API = ENABLE_CSS_PAINT_API;

The spec calls this the "CSS Painting API"

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:625
> -		1C09D0591E31C6A900725F18 /* libPAL.a in Frameworks */ = {isa = PBXBuildFile; fileRef = 1C09D0501E31C32900725F18 /* libPAL.a */; };
> +		1C09D0591E31C6A900725F18 /* lib.a in Frameworks */ = {isa = PBXBuildFile; fileRef = 1C09D0501E31C32900725F18 /* lib.a */; };

Whut.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:16546
> -				1C09D0501E31C32900725F18 /* libPAL.a */,
> +				1C09D0501E31C32900725F18 /* lib.a */,

Noooo

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:31621
> -		1C09D0501E31C32900725F18 /* libPAL.a */ = {
> +		1C09D0501E31C32900725F18 /* lib.a */ = {
>  			isa = PBXReferenceProxy;
>  			fileType = archive.ar;
> -			path = libPAL.a;
> +			path = lib.a;

Bad

> Source/WebCore/bindings/js/JSCSSPaintWorkletGlobalScopeCustom.cpp:26
> +#if ENABLE(CSS_PAINT_API)

This should to after config.h and the main header.

> Source/WebCore/css/CSSPaintWorkletGlobalScope.cpp:25
> +#if ENABLE(CSS_PAINT_API)

Move this below the #includes. Blank line after the license.

> Source/WebCore/css/CSSPaintWorkletGlobalScope.h:25
> +#if ENABLE(CSS_PAINT_API)

Blank line above.

> Source/WebKit/Shared/WebPreferences.yaml:1310
> +  humanReadableDescription: "Enable CSS Painting API"

Enable the ?

> Source/WebKit/WebProcess/InjectedBundle/InjectedBundle.cpp:255
> +    if (preference == "CSSCustomPaintEnabled")

Should "CSSCustomPaintEnabled" be "CSSPaintingAPI"?

[Justin Michaud]

Created attachment 352434 [details]
Patch

[Justin Michaud]

Created attachment 352437 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 352437 [details]
Patch

Let's wait for EWS to go green for cq.

[Justin Michaud]

Created attachment 352438 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 352438 [details]
Patch

Clearing flags on attachment: 352438

Committed r237192: <https://trac.webkit.org/changeset/237192>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45311644>