search

w3c / media-source

Media Source Extensions
https://w3c.github.io/media-source/
Other
268 stars 57 forks source link

Exposing createObjectURL(mediaSource) in workers seems odd #168

Closed foolip closed 7 years ago

foolip commented 7 years ago

Since there can be no MediaSource instance in a worker, why have the overload there? Is it that WebIDL requires overloads to all have the same exposure set?

@inexorabletash

paulbrucecotton commented 7 years ago

@foolip: Can you give us an exact reference to the material in the MSE spec that this issue is about?

@wolenetz: Is this related to the solution we applied to ISSUE-67?

foolip commented 7 years ago

It's https://w3c.github.io/media-source/#url and the [Exposed=Window, DedicatedWorker, SharedWorker] in the IDL. This matches https://w3c.github.io/FileAPI/#creating-revoking but is it intentional or just copy-pasted? In https://heycam.github.io/webidl/#Exposed I see nothing about the exposure set of a partial interface having to match the exposure set of the interface itself.

Ping @inexorabletash.

inexorabletash commented 7 years ago

I probably provided bad guidance here. IIRC it was [Exposed=(Window,Worker)] and we wanted to ensure that none of the Blob-URL-minting functions were exposed to Service Workers. I probably suggested that this be scoped to specific workers (minimal change) rather than just window (correct change) due to not thinking about the feature itself.

foolip commented 7 years ago

Cool, so the fix here is to just make it [Exposed=Window]. Thanks @inexorabletash!

wolenetz commented 7 years ago

To be clear, if we continued to (probably mistakenly) allow createObjectURL within a DedicatedWorker or a SharedWorker, in what context might such a URL even be assignable to an HTMLMediaElement's src attribute? Are there similar exposure restrictions on any of HTMLMediaElement operation?

foolip commented 7 years ago

It's actually harmless since there's no way to get a MediaSource instance in a worker, and thus no way to generate such a URL. So consider this editorial.

wolenetz commented 7 years ago

SGTM. I'll prepare a pull request shortly, though I'll need @plehegar to help determine how exactly such a change might make it (is it a post-REC erratum, or something to include for REC; and either way, should this be on mainline gh-pages here?)

@plehegar, a second question: we currently have a registries-publication branch that is a few commits ahead of gh-pages, should we just fast-forward gh-pages to that, or is that branch intentional since it's expected that registry changes can occur without impacting main v1 spec in future?

wolenetz commented 7 years ago

@jdsmith3000 @paulbrucecotton @mwatson2 Also, pending @plehegar's response to my questions, above, triaging this bug to a particular Milestone is TBD.

plehegar commented 7 years ago

My understanding is that the change is editorial, so I'd expect to include the change as part of the Proposed Recommendation.

plehegar commented 7 years ago

re registries, I created the branch for pull request purpose. I'm fine with synchronizing and remove the branch.

bzbarsky commented 6 years ago

Is it that WebIDL requires overloads to all have the same exposure set?

In fact, it does. From https://heycam.github.io/webidl/#Exposed

If [Exposed] appears on an overloaded operation, then it must appear identically on all overloads.

because exposure controls whether the property appears at all, and there is only one property representing all the overloads.

bzbarsky commented 6 years ago

So to be clear:

  1. This change made the IDL invalid.
  2. This IDL was invalid to start with because it's adding overloads across partial interfaces, which WebIDL also prohibits....
foolip commented 6 years ago

Thanks @bzbarsky! And thanks @saschanaz for filing https://github.com/w3c/media-source/issues/211 and https://github.com/w3c/FileAPI/issues/100 to fix this.