-
Notifications
You must be signed in to change notification settings - Fork 142
[css-paint-api]: APIs not exposed in Worklets #237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
Needs to be exposed only to 'PaintWorklet' not the more generic 'Worklet' (exposing this to 'AudioWorklet' won't make sense). |
Yeah, presumably they'll be added to other CSS worklets too, but I suppose you can refactor when that happens. |
I don't think there needs to be any specific spec text in css-typed-om to support this, so I'm removing the typed-om tag. |
This needs the |
Can you provide a list of the things you want exposed? |
There is the open question of #238 as well. |
We resolved in #238 not to expose constructors and parse methods. I think that contradicts this issue? |
No it doesn't. Even if you don't expose some things, doesn't mean you'd expose nothing. |
if Ian's list including CSSStyleValue, CSSKeywordValue, CSSTransformComponent & subclasses is asking for the constructors of these objects to be exposed, that would seem to conflict with "RESOLVED: don't expose constructor and parse methods". Maybe I'm missing something? |
@shans constructors and parse methods are not all there is to an object. So if some parts of the objects do need to be exposed, you'll need these annotations. And it seems to me you do want to expose certain things in paint worklets. |
subclasses of the CSSStyleValue hierarchy provide parse methods. Is it possible to expose only part of an interface? Is that something we tend to want to do? e.g. CSSNumericValue exposes a static parse: https://drafts.css-houdini.org/css-typed-om-1/#numeric-objects |
Yes. The whole point of [Exposed] is that you can do it on a per-interface-member basis as desired. |
OK. I talked to Tab about it too and he's convinced me it's not that weird. |
This CL makes CSSStyleValue and StylePropertyMapReadOnly to be exposed to CSSTypedOM on Window, and CSSPaintAPI on PaintWorklet. It also restricts the parse API in CSSStyleValue to be exposed to Window only. The discussion for the change to CSSStyleValue and StylePropertyMapReadOnly is here: w3c/css-houdini-drafts#237. The discussion for keep the parse API exposed to Window only is here: w3c/css-houdini-drafts#238 Bug: 728597 Change-Id: I489571047829997e5a523f03a4861dbb9737d914 Reviewed-on: https://chromium-review.googlesource.com/521945 Commit-Queue: Xida Chen <[email protected]> Reviewed-by: Kentaro Hara <[email protected]> Reviewed-by: Yuki Shiino <[email protected]> Reviewed-by: Eddy Mead <[email protected]> Reviewed-by: Ian Kilpatrick <[email protected]> Cr-Commit-Position: refs/heads/master@{#478289}
"In addition, for every [NamedConstructor] extended attribute on an exposed interface, a corresponding property must exist on the ECMAScript global object." - putting [Exposed] on an interface also exposes the constructors. "If [Exposed] appears an interface member, then the interface member’s exposure set must be a subset of the exposure set of the interface or partial interface it’s a member of" - you can't expose members of an interface without exposing the interface itself. So we can hide parse methods by putting a more restrictive [Exposed] extended attribute on them, but we can't hide the constructors, at least not with Exposed. Two alternatives:
I vote for (1) |
You can expose the constructors, but just make them throw based on the "current global object" (defined by HTML). |
That would work. However there's no actual harm in allowing the constructors to exist in the Worklets, we just couldn't think of a use-case to have them. If they're going to be there anyway I'd rather that they just construct the objects, as throwing would be a weird API affordance. |
Well, the issue you duplicated against this that I filed raised a number of issues with various value types and parsers. |
I can't see major unaddressed issues in #238? The main arguments I can see are:
|
I think it rather depends on your implementation strategy. If the worklet is in a different thread it might well help to not expose certain functionality there and you might opt to have different backing classes. One that supports parsing and one available in worklets that doesn't. |
OK, we can bring this up at the next F2F and get a sense for implementer positions. |
It's REALLY bad form to expose objects but not the constructor for the interface, because then you can't use instanceof or easily reach the prototype object and so forth. So if you plan to expose the objects at all, you should have the constructor exposed. The only question is what it does when called. |
The big thing we want to avoid is requiring the CSS parser to be active in worklets; we don't have a thread-safe version yet. Luckily, we've moved away from the parsing-based constructors anyway; they're all factory methods now, and can be individually marked as not exposed. The only thing that does parsing is for URLs, and Chrome definitely has a thread-safe URL parser (needed for workers of all kinds), and I assume everyone else does too. |
You shouldn't assume things. For example, Gecko does not have a threadsafe URL parser (though we're working on changing that; it's complicated because it's pluggable by extensions). It just proxies to a single thread for all URL parsing in workers. |
Oh, and Gecko does, with stylo, have a threadsafe CSS parser. So again, please don't make the assumption that Blink's architectural constraints are in any way universal. |
@bzbarsky point taken. I think Tab's point was more that browsers have to URL parse for worker resources anyway, so parsing URLs as a result of an action in a separate thread is not new (as opposed to parsing CSS as a result of an action in a separate thread, which is not currently a feature of the open web). Coming back to the issue, the two viable options are: In neither case is CSS parse functionality exposed, based on WG consensus. I lean towards (2) because it's better ergonomically for developers (less surprising). I will also assert that no weight is saved by having throwing constructors over functional constructors. If URL parsing is problematic for Gecko then we can always move the parsing stage for resource values to the transition between unloaded and pending. Are there any other considerations that should be taken into account? |
I don't think you should be designing this API around Gecko's current limitations, just like you shouldn't be designing it around Blink's. Design it in the way that makes the most sense for consumers, unless you get feedback that that's flat-out not implementable for some reason. |
FWIW, if they're not going to work I'd rather have the constructors throw. That makes it easier to feature detect when they start working (if we ever decide to want that). |
Again, we don't have any constructors that require CSS parsing. All the constructors take the relevant data directly, in JS datatypes. CSS parsing is done in factory methods instead. We can exclude those for now, as most browser have trouble with this, and expose them later. URL parsing is already done in workers somehow (whether with a proxy to a dedicated thread, or with a thread-safe parser), so the constructors that require URL parsing are kosher. Shane jumped a bit in the topic, which has confused both Boris and Anne, but the only other "problematic" constructors are those that fetch an external resource. We don't want to allow an external communication channel like that in worklets, so for these objects, we're defining that if you construct one in a worklet, it sticks in an "unloaded" state while it "lives" in the worklet. (It can start loading later, after it's been set to some style and the main thread has taken over.) A detail of that is, if Gecko's URL-parsing-thru-thread-proxy is troublesome to do in worklets, we're okay with moving the URL parsing slightly later in the construction algorithm, so that it doesn't occur until the object attempts to switch to the "pending" state (at which point, if the URL is invalid, it'll instead switch to the "error" state). Does Gecko have a preference here? |
Again, I think you should design the API in the way that makes the most sense to consumers. Gecko may well end up with off-thread URL parsing before we actually decide whether we're even implementing this API. |
So for Typed OM, all that needs to be done is to mark that the various .parse() methods need to be marked as only available in documents, not workers. The fact that resource-fetching objects shouldn't fetch their resources when constructed in non-document contexts is already captured in #186. And that's all that TypedOM needs, I think, since nothing else invokes a verboten parser. |
@nainar the second reference there is this issue... |
Plus the latest commit I just added, which adds [Exposed] to every interface (and the appropriate methods) in the draft. |
@tabatkins @nainar This is now fixed right? |
Yeah, looks like paint-api is fixed now, and it was the last one. |
It seems we want the interfaces in https://drafts.css-houdini.org/css-typed-om/ to be exposed in Worklets, right?
@bzbarsky
The text was updated successfully, but these errors were encountered: