Bug 190237 - Implement feature flag and bindings for CSS Painting API
Summary: Implement feature flag and bindings for CSS Painting API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Michaud
URL:
Keywords: InRadar
Depends on:
Blocks: 190217
  Show dependency treegraph
 
Reported: 2018-10-02 23:10 PDT by Justin Michaud
Modified: 2018-10-16 11:05 PDT (History)
8 users (show)

See Also:


Attachments
Patch (44.82 KB, patch)
2018-10-02 23:20 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.32 MB, application/zip)
2018-10-03 00:31 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-sierra (3.09 MB, application/zip)
2018-10-03 01:27 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews205 for win-future (12.69 MB, application/zip)
2018-10-03 08:19 PDT, Build Bot
no flags Details
Patch (46.78 KB, patch)
2018-10-03 10:05 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (57.93 KB, patch)
2018-10-04 18:50 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (58.17 KB, patch)
2018-10-04 21:10 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.37 MB, application/zip)
2018-10-04 22:24 PDT, Build Bot
no flags Details
Patch (59.01 KB, patch)
2018-10-04 22:35 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (59.45 KB, patch)
2018-10-15 13:31 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (59.82 KB, patch)
2018-10-15 14:04 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (59.83 KB, patch)
2018-10-15 14:30 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (101.93 KB, patch)
2018-10-15 16:12 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (104.72 KB, patch)
2018-10-15 16:55 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (102.23 KB, patch)
2018-10-15 21:05 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (102.24 KB, patch)
2018-10-15 21:28 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (102.48 KB, patch)
2018-10-15 22:26 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Michaud 2018-10-02 23:10:39 PDT
Implement a feature flag for CSS Painting API
Comment 1 Justin Michaud 2018-10-02 23:20:10 PDT
Created attachment 351482 [details]
Patch
Comment 2 Build Bot 2018-10-02 23:23:03 PDT Comment hidden (obsolete)
Comment 3 Build Bot 2018-10-03 00:31:51 PDT Comment hidden (obsolete)
Comment 4 Build Bot 2018-10-03 00:31:52 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2018-10-03 01:27:37 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2018-10-03 01:27:39 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2018-10-03 08:19:26 PDT Comment hidden (obsolete)
Comment 8 Build Bot 2018-10-03 08:19:37 PDT Comment hidden (obsolete)
Comment 9 Justin Michaud 2018-10-03 10:05:43 PDT
Created attachment 351530 [details]
Patch
Comment 10 Build Bot 2018-10-03 10:08:38 PDT Comment hidden (obsolete)
Comment 11 Ryosuke Niwa 2018-10-03 19:46:00 PDT
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.
Comment 12 Justin Michaud 2018-10-04 18:50:34 PDT
Created attachment 351648 [details]
Patch
Comment 13 Ryosuke Niwa 2018-10-04 20:08:21 PDT
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.
Comment 14 Justin Michaud 2018-10-04 21:10:09 PDT
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"
Comment 15 Justin Michaud 2018-10-04 21:10:27 PDT
Created attachment 351651 [details]
Patch
Comment 16 Build Bot 2018-10-04 22:24:31 PDT Comment hidden (obsolete)
Comment 17 Build Bot 2018-10-04 22:24:33 PDT Comment hidden (obsolete)
Comment 18 Justin Michaud 2018-10-04 22:35:34 PDT
Created attachment 351655 [details]
Patch
Comment 19 Ryosuke Niwa 2018-10-05 21:22:30 PDT
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!
Comment 20 Justin Michaud 2018-10-15 11:17:48 PDT
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.
Comment 21 Justin Michaud 2018-10-15 11:26:31 PDT
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.
Comment 22 Justin Michaud 2018-10-15 13:31:21 PDT
Created attachment 352367 [details]
Patch
Comment 23 Justin Michaud 2018-10-15 14:04:01 PDT
Created attachment 352374 [details]
Patch
Comment 24 Justin Michaud 2018-10-15 14:30:39 PDT
Created attachment 352381 [details]
Patch
Comment 25 Justin Michaud 2018-10-15 16:12:52 PDT
Created attachment 352398 [details]
Patch
Comment 26 Justin Michaud 2018-10-15 16:55:48 PDT
Created attachment 352405 [details]
Patch
Comment 27 Simon Fraser (smfr) 2018-10-15 18:44:46 PDT
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"?
Comment 28 Justin Michaud 2018-10-15 21:05:02 PDT
Created attachment 352434 [details]
Patch
Comment 29 Justin Michaud 2018-10-15 21:28:31 PDT
Created attachment 352437 [details]
Patch
Comment 30 Ryosuke Niwa 2018-10-15 21:43:51 PDT
Comment on attachment 352437 [details]
Patch

Let's wait for EWS to go green for cq.
Comment 31 Justin Michaud 2018-10-15 22:26:16 PDT
Created attachment 352438 [details]
Patch
Comment 32 WebKit Commit Bot 2018-10-16 11:04:51 PDT
Comment on attachment 352438 [details]
Patch

Clearing flags on attachment: 352438

Committed r237192: <https://trac.webkit.org/changeset/237192>
Comment 33 WebKit Commit Bot 2018-10-16 11:04:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 Radar WebKit Bug Importer 2018-10-16 11:05:49 PDT
<rdar://problem/45311644>