Bug 190237

Summary:

Implement feature flag and bindings for CSS Painting API

Product:

WebKit

Reporter:

Justin Michaud <justin_michaud>

Component:

Layout and Rendering

Assignee:

Justin Michaud <justin_michaud>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, commit-queue, ews-watchlist, rniwa, simon.fraser, webkit-bug-importer, wenson_hsieh, zalan

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

190217

Attachments:

Description	Flags
Patch	none
Archive of layout-test-results from ews102 for mac-sierra	none
Archive of layout-test-results from ews115 for mac-sierra	none
Archive of layout-test-results from ews205 for win-future	none
Patch	none
Patch	none
Patch	none
Archive of layout-test-results from ews102 for mac-sierra	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Justin Michaud

Reported 2018-10-02 23:10:39 PDT

Implement a feature flag for CSS Painting API

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, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews115 for mac-sierra (3.09 MB, application/zip) 2018-10-03 01:27 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews205 for win-future (12.69 MB, application/zip) 2018-10-03 08:19 PDT, EWS Watchlist	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, EWS Watchlist	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
Show Obsolete (16) View All Add attachment proposed patch, testcase, etc.

Justin Michaud

Comment 1 2018-10-02 23:20:10 PDT

Created attachment 351482 [details] Patch

EWS Watchlist

Comment 2 2018-10-02 23:23:03 PDT Comment hidden (obsolete)

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 3 2018-10-03 00:31:51 PDT Comment hidden (obsolete)

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

Comment 4 2018-10-03 00:31:52 PDT Comment hidden (obsolete)

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 5 2018-10-03 01:27:37 PDT Comment hidden (obsolete)

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

Comment 6 2018-10-03 01:27:39 PDT Comment hidden (obsolete)

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 7 2018-10-03 08:19:26 PDT Comment hidden (obsolete)

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

Comment 8 2018-10-03 08:19:37 PDT Comment hidden (obsolete)

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

Comment 9 2018-10-03 10:05:43 PDT

Created attachment 351530 [details] Patch

EWS Watchlist

Comment 10 2018-10-03 10:08:38 PDT Comment hidden (obsolete)

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 11 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.

Justin Michaud

Comment 12 2018-10-04 18:50:34 PDT

Created attachment 351648 [details] Patch

Ryosuke Niwa

Comment 13 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.

Justin Michaud

Comment 14 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"

Justin Michaud

Comment 15 2018-10-04 21:10:27 PDT

Created attachment 351651 [details] Patch

EWS Watchlist

Comment 16 2018-10-04 22:24:31 PDT Comment hidden (obsolete)

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

Comment 17 2018-10-04 22:24:33 PDT Comment hidden (obsolete)

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

Comment 18 2018-10-04 22:35:34 PDT

Created attachment 351655 [details] Patch

Ryosuke Niwa

Comment 19 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> > +<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> > +<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 20 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.

Justin Michaud

Comment 21 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.

Justin Michaud

Comment 22 2018-10-15 13:31:21 PDT

Created attachment 352367 [details] Patch

Justin Michaud

Comment 23 2018-10-15 14:04:01 PDT

Created attachment 352374 [details] Patch

Justin Michaud

Comment 24 2018-10-15 14:30:39 PDT

Created attachment 352381 [details] Patch

Justin Michaud

Comment 25 2018-10-15 16:12:52 PDT

Created attachment 352398 [details] Patch

Justin Michaud

Comment 26 2018-10-15 16:55:48 PDT

Created attachment 352405 [details] Patch

Simon Fraser (smfr)

Comment 27 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"?

Justin Michaud

Comment 28 2018-10-15 21:05:02 PDT

Created attachment 352434 [details] Patch

Justin Michaud

Comment 29 2018-10-15 21:28:31 PDT

Created attachment 352437 [details] Patch

Ryosuke Niwa

Comment 30 2018-10-15 21:43:51 PDT

Comment on attachment 352437 [details] Patch Let's wait for EWS to go green for cq.

Justin Michaud

Comment 31 2018-10-15 22:26:16 PDT

Created attachment 352438 [details] Patch