-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Treat blob: URLs as local in the resource selection algorithm #1037
Conversation
I looked at many ways of doing this, and it looked tempting to eliminate the mode variable and just use current media resource, but in the end I went with the simplest possible thing that matched option 2 from #295 (comment) @zcorpan, review? |
AFAICT this won't invoke "fetch" of the blob: URL. I suppose that's necessary? |
What are the correct words from checking if a URL corresponds to an object and for getting that object? In Blink's implementation it doesn't have anything to do with fetching, and it's a synchronous operation. I guess https://w3c.github.io/FileAPI/#requestResponseModel should cover this, but if one expresses it in terms of an async fetch then the structure will be very different. https://w3c.github.io/FileAPI/#BlobURLStore is a primitive which does support synchronous operations, but there's no synchronous "get an entry from the Blob URL Store". Thoughts? |
@annevk can you help here? |
In https://codereview.chromium.org/1881733004/ (by @wolenetz) it looks like implementation-wise it's more straightforward to just check if it's a blob: URL and not care whether it corresponds to an object at that point. Depending on how one correctly gets from a blob: URL to an actual object (@annevk?) it might make more sense to keep blob: URLs in the remote branch and instead just skip the preload check, which is what the Chromium CL does. |
If you have a parsed URL (a URL record), it'll have an object property to it pointing to the Blob object. See https://url.spec.whatwg.org/#url-parsing for details. |
Right now it seems there is confusion in the spec between URL string and URL record.
and in the resource fetch algorithm
But "creating a potential-CORS request" takes a URL record, not a URL string. So we need to change the spec here to work with the URL record instead. (Similar in the |
Yeah, I have not tried to correct that yet. Baby steps, but if you can do some of it that'd be great. |
@annevk OK I tried to fix that here, PTAL. |
The changes that commit make look reasonable. I'm not a big fan of the "would have" style, but that was already there. |
Regarding #1037 (comment), I'm not clear on whether or not MSE or other blob/object URLs would best be handled in the whatwg html5 spec as "remote" or "local" mode, but at least MSE and MediaStream blob/object URLs need to skip the "remote" mode "optional preload none" steps. |
I agree that the "would have" bits are silly, that's #798 @zcorpan's changes LGTM. The one point of uncertainty now is that in https://codereview.chromium.org/1881733004/ the preload check will be skipped for any URL using the |
@annevk, final check before I merge? I'll combine the two commits. |
I think the thing that is missing is that later on you don't actually use the object given by the URL. Fetch has a way of turning that into a response, but it seems that for the local mode you don't invoke fetch. |
51ebdde
to
b0a43d9
Compare
OK, I've fixed a build error and checked the diff again.
"otherwise, let the current media resource be the resource given by the media provider object" in the following step implicitly refers to that object. Do we really need to fetch anything when we already have the object we want? The implementation wouldn't I think. A problem, however, is that https://url.spec.whatwg.org/#concept-url-object can only be |
Yeah, I didn't know it had spread beyond You need to read the object somehow. Maybe the implicitness is okay for now though. |
Anyway, LGTM if you file that issue, although I think at some point we probably want to be more explicit how we read objects and such. |
Filed whatwg/url#126 |
b0a43d9
to
f8bc379
Compare
Fixes #295.