[webkit-reviews] review granted: [Bug 190237] Implement feature flag and bindings for CSS Painting API : [Attachment 351655] Patch

Fri Oct 5 21:22:30 PDT 2018

Ryosuke Niwa <rniwa at webkit.org> has granted Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 190237: Implement feature flag and bindings for CSS Painting API
https://bugs.webkit.org/show_bug.cgi?id=190237

Attachment 351655: Patch

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

--- Comment #19 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 351655
  --> https://bugs.webkit.org/attachment.cgi?id=351655
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/D
OMImplementation-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!