Paint worklets: Mark nondeterministic registration as invalid

[asajeffrey]

w3c/css-houdini-drafts#423

[nox]

Where is the code where that should be done?

[asajeffrey]

In the code

servo/components/layout_thread/lib.rs

Line 748 in e3624e6

self.registered_painters.0.insert(name, registered_painter);

Currently we just insert a new registered painter. Really we should check to see if there already is one, and if there is check to make sure its attributes are the same. If they're not, remove the existing definition and replace it by a tombstone.

[kafji]

Can I give this a try?

[asajeffrey]

Go for it!

[KiChjang]

@kafji Are you still working on this?

[kafji]

@KiChjang Yes

[kafji]

If they're not, remove the existing definition and replace it by a tombstone.

@asajeffrey What is tombstone?

[jdm]

I believe it means a value that cannot be used, but represents a value that used to be usable.

[asajeffrey]

Indeed, an object representing a failed attempt to register a paint worklet.

[KiChjang]

@kafji Is this still being worked on?

[kafji]

No

…

On Wed, 29 Nov 2017, 09:38 Keith Yeung, ***@***.***> wrote: @kafji <https://github.com/kafji> Is this still being worked on? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17584 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABwb3zfq4oyNDas5Eg_kE5bUL5eLX-Xkks5s7MOrgaJpZM4OLJRZ> .

[wabain]

I'm interested in working on this, assuming that nothing's changed in the past few months, but I have a few questions:

The spec says that equivalency of paint definitions is determined by equality of "input properties, input argument syntaxes, and PaintRenderingContext2DSettings object". However, currently only the input properties are sent to the layout thread, and parsing the input arguments is left as a TODO in the paint worklet global scope. Does it make sense to do that parsing and then send those parameters to the layout thread for this validation?

Also, the spec says that if the registrations don't match the UA should:

Log an error to the debugging console stating that the same class was registered with different inputProperties, inputArguments, or paintRenderingContext2DSettings.

How would that be done?

[jdm]

@asajeffrey Can answer the first question, but the logging can just use println! for the time being.

[asajeffrey]

@wabain probably just easiest to treat the properties as strings, and ignore the arguments. If you want to add support for the argument syntaxes, that would be great, but it probably means implementing a lot more of the typed CSS object model!

[wabain]

Sounds good! I'm opening a WIP PR with a first-pass implementation. There's still some work needed to invalidate the document paint definition if a paint worklet finds that the painter wasn't ever registered in its scope. I also don't know if there's a good way to add a deterministic reftest for this, or if there's a threshold where a fallible reftest is good enough (e.g. if it will produce a conflict > 99% of the time).

[asajeffrey]

@wabain yes, testing this is going to be painful, since we'd need to mock up a worklet that is guaranteed to execute differently on different loads. The only way I can think of doing this is to use the wpt http server to implement a counter, then have the JS code increment the counter, and register different things depending on the value of the counter.

[wabain]

I gave this a try and it looks like fetch and xhr aren't exposed in worklet scope. Assuming that's by design, maybe I could add a debug option to expose a counter in the worklet scope?

[asajeffrey]

Oh that's annoying. We could have the http server return a different JS program based on the counter it maintains? Otherwise, as you said, we could add a just-for-testing global counter (under the hood it would be something like a Arc<AtomicUsize>).

We could also use high-precision timers, and rely on the chance of two timers returning the same value as being vanishingly small.

[wabain]

The module is only actually fetched from the server once, so unfortunately we can't vary the response. It would also be good to test some binary states (e.g. painter registered / not registered, alpha true / alpha false), and I don't know of a scheme that would give really strong odds of getting multiple outputs in those cases from timing information. I think it will have to be a test timer.

[asajeffrey]

Oh that's annoying, our http cache is getting in the way :/

Probably the easiest thing is the timer then. or perhaps math.random()? Googling stack overflow for JS uuid gives: https://stackoverflow.com/a/2117523