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: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||||||||||
Bug Blocks: | 190217 | ||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Justin Michaud
2018-10-02 23:10:39 PDT
Created attachment 351482 [details]
Patch
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.
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 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
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 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
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 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
Created attachment 351530 [details]
Patch
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.
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. Created attachment 351648 [details]
Patch
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 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" Created attachment 351651 [details]
Patch
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 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
Created attachment 351655 [details]
Patch
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 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. 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. Created attachment 352367 [details]
Patch
Created attachment 352374 [details]
Patch
Created attachment 352381 [details]
Patch
Created attachment 352398 [details]
Patch
Created attachment 352405 [details]
Patch
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"? Created attachment 352434 [details]
Patch
Created attachment 352437 [details]
Patch
Comment on attachment 352437 [details]
Patch
Let's wait for EWS to go green for cq.
Created attachment 352438 [details]
Patch
Comment on attachment 352438 [details] Patch Clearing flags on attachment: 352438 Committed r237192: <https://trac.webkit.org/changeset/237192> All reviewed patches have been landed. Closing bug. |