Skip to content
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

[css-paint-api]: APIs not exposed in Worklets #237

Closed
annevk opened this issue Jun 22, 2016 · 36 comments
Closed

[css-paint-api]: APIs not exposed in Worklets #237

annevk opened this issue Jun 22, 2016 · 36 comments

Comments

@annevk
Copy link
Member

annevk commented Jun 22, 2016

It seems we want the interfaces in https://drafts.css-houdini.org/css-typed-om/ to be exposed in Worklets, right?

@bzbarsky

@shans shans changed the title CSS Typed: APIs not exposed in Worklets [css-typed-om]: APIs not exposed in Worklets Jun 24, 2016
@bfgeek
Copy link
Contributor

bfgeek commented Jun 24, 2016

Needs to be exposed only to 'PaintWorklet' not the more generic 'Worklet'

(exposing this to 'AudioWorklet' won't make sense).

@annevk
Copy link
Member Author

annevk commented Jun 25, 2016

Yeah, presumably they'll be added to other CSS worklets too, but I suppose you can refactor when that happens.

@tabatkins tabatkins self-assigned this Aug 11, 2016
@shans shans changed the title [css-typed-om]: APIs not exposed in Worklets [css-typed-om][css-paint-api]: APIs not exposed in Worklets Aug 11, 2016
@shans
Copy link
Contributor

shans commented Apr 3, 2017

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.

@shans shans changed the title [css-typed-om][css-paint-api]: APIs not exposed in Worklets [css-paint-api]: APIs not exposed in Worklets Apr 3, 2017
@bfgeek
Copy link
Contributor

bfgeek commented Apr 6, 2017

This needs the [Exposed=PaintWorklet] annotation in the typed-om spec for all the things we want exposed there.

@shans
Copy link
Contributor

shans commented Apr 12, 2017

Can you provide a list of the things you want exposed?

@bfgeek
Copy link
Contributor

bfgeek commented Apr 16, 2017

CSSStyleValue and of the sub-classes, e.g. CSSKeywordValue etc.
CSSTransformComponent and sub-classes.
StylePropertyMapReadOnly (...no need for StylePropertyMap).

There is the open question of #238 as well.

@shans
Copy link
Contributor

shans commented May 23, 2017

We resolved in #238 not to expose constructors and parse methods. I think that contradicts this issue?

@annevk
Copy link
Member Author

annevk commented May 23, 2017

No it doesn't. Even if you don't expose some things, doesn't mean you'd expose nothing.

@shans
Copy link
Contributor

shans commented May 24, 2017

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?

@annevk
Copy link
Member Author

annevk commented May 24, 2017

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

@shans
Copy link
Contributor

shans commented May 25, 2017

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

@bzbarsky
Copy link

Is it possible to expose only part of an interface?

Yes. The whole point of [Exposed] is that you can do it on a per-interface-member basis as desired.

@shans
Copy link
Contributor

shans commented May 25, 2017

OK. I talked to Tab about it too and he's convinced me it's not that weird.

MXEBot pushed a commit to mirror/chromium that referenced this issue Jun 10, 2017
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 <xidachen@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Eddy Mead <meade@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#478289}
@shans
Copy link
Contributor

shans commented Jul 25, 2017

"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:

  1. just expose the constructors. It's fine.
  2. work out some [Global] hack that lets us have two different versions of the same objects in the different contexts.

I vote for (1)

@annevk
Copy link
Member Author

annevk commented Jul 25, 2017

You can expose the constructors, but just make them throw based on the "current global object" (defined by HTML).

@shans
Copy link
Contributor

shans commented Jul 25, 2017

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.

@annevk
Copy link
Member Author

annevk commented Jul 25, 2017

Well, the issue you duplicated against this that I filed raised a number of issues with various value types and parsers.

@shans
Copy link
Contributor

shans commented Jul 25, 2017

I can't see major unaddressed issues in #238? The main arguments I can see are:

  • worklets might be a little more lightweight without constructors / parse methods. Having constructors present but throwing seems just as bad as having full constructors in this regard, as the implementations are going to be native, not JS.
  • constructing images specifically from URLs couldn't result in a resolved image. We proposed that resources constructed like this could be permanently marked as unloaded, which is a more graceful degradation than throwing IMO.

@annevk
Copy link
Member Author

annevk commented Jul 25, 2017

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.

@shans
Copy link
Contributor

shans commented Jul 25, 2017

OK, we can bring this up at the next F2F and get a sense for implementer positions.

@bzbarsky
Copy link

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.

@tabatkins
Copy link
Member

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.

@bzbarsky
Copy link

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.

@bzbarsky
Copy link

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.

@shans
Copy link
Contributor

shans commented Jul 27, 2017

@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:
(1) expose StyleValue objects with constructors that always throw
(2) expose StyleValue objects with constructors that work; however those requiring resource loading never get past the unloaded stage.

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?

@bzbarsky
Copy link

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.

@annevk
Copy link
Member Author

annevk commented Jul 27, 2017

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

@tabatkins
Copy link
Member

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?

@bzbarsky
Copy link

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.

@tabatkins
Copy link
Member

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
Copy link
Contributor

nainar commented Jan 29, 2018

This is referenced in issues #549 and #237. So removing the typed om label from this one.

@annevk
Copy link
Member Author

annevk commented Jan 29, 2018

@nainar the second reference there is this issue...

@nainar
Copy link
Contributor

nainar commented Jan 29, 2018

Sorry I meant #549 and #237 (this issue) are solved by both of the following PRs:
#430
#597

tabatkins added a commit that referenced this issue Jan 31, 2018
…g that needs parsing is restricted to Window. Related to #237. Closes #430.
@tabatkins
Copy link
Member

Plus the latest commit I just added, which adds [Exposed] to every interface (and the appropriate methods) in the draft.

@bfgeek
Copy link
Contributor

bfgeek commented Apr 5, 2018

@tabatkins @nainar This is now fixed right?

@tabatkins
Copy link
Member

Yeah, looks like paint-api is fixed now, and it was the last one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants