Adding window.URL.createObjectURL(MediaStream) with associated supporting code.
Created attachment 103367 [details] Patch
Comment on attachment 103367 [details] Patch Attachment 103367 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9340230 New failing tests: fast/files/create-blob-url-crash.html
Created attachment 103375 [details] Patch
Comment on attachment 103375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103375&action=review > LayoutTests/fast/files/create-blob-url-crash-expected.txt:2 > -PASS: Not enough arguments > +PASS: Type error Why is this exception changing type? I suspect we need to change the code generator to check the number of arguments before it checks the type of the arguments. > Source/WebCore/dom/ScriptExecutionContext.cpp:396 > + publicURL.setProtocol("x-raw-media"); Where does this URI scheme come from? It is registered? > Source/WebCore/dom/ScriptExecutionContext.cpp:397 > + // Since WebWorkers cannot obtain Stream objects, we should be on the main thread. Maybe add an ASSERT(isMainThread()) ? > Source/WebCore/dom/ScriptExecutionContext.cpp:424 > + // Since WebWorkers cannot obtain Stream objects, we should be on the main thread. Same here. > Source/WebCore/platform/MediaStreamRegistry.cpp:55 > +PassRefPtr<MediaStream> MediaStreamRegistry::getStream(const KURL& url) const getStream -> WebKit doesn't use a leading "get" in function names.
Comment on attachment 103375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103375&action=review >> LayoutTests/fast/files/create-blob-url-crash-expected.txt:2 >> +PASS: Type error > > Why is this exception changing type? I suspect we need to change the code generator to check the number of arguments before it checks the type of the arguments. No idea as to why the code the code generator acts differently now, but I have verified that the JS bindings throw a type error as well. >> Source/WebCore/dom/ScriptExecutionContext.cpp:396 >> + publicURL.setProtocol("x-raw-media"); > > Where does this URI scheme come from? It is registered? I guess the people responsible for the chromium <video> tag wanted to somehow differentiate between "raw" video (from webcam) and other video data, but I was not involved in the discussion. >> Source/WebCore/dom/ScriptExecutionContext.cpp:397 >> + // Since WebWorkers cannot obtain Stream objects, we should be on the main thread. > > Maybe add an ASSERT(isMainThread()) ? Done. >> Source/WebCore/dom/ScriptExecutionContext.cpp:424 >> + // Since WebWorkers cannot obtain Stream objects, we should be on the main thread. > > Same here. Done. >> Source/WebCore/platform/MediaStreamRegistry.cpp:55 >> +PassRefPtr<MediaStream> MediaStreamRegistry::getStream(const KURL& url) const > > getStream -> WebKit doesn't use a leading "get" in function names. Done, and I renamed the functions too to make them clearer (stream -> mediaStream).
Created attachment 103471 [details] Patch
Created attachment 103476 [details] Patch
Created attachment 103488 [details] Patch
Comment on attachment 103488 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103488&action=review > LayoutTests/fast/files/create-blob-url-crash-expected.txt:2 > -PASS: Not enough arguments > +PASS: Type error IMHO, we shouldn't change this behavior. We probably need to fix the code generator to generate correct code. > Source/WebCore/ChangeLog:7 > + MediaStream API: add createObjectURL functionality > + https://bugs.webkit.org/show_bug.cgi?id=65925 > + > + Reviewed by NOBODY (OOPS!). > + This ChangeLog doesn't explain why we're making this change or why this patch doesn't contain tests. Is there some reason we can't test this patch? > Source/WebCore/dom/ScriptExecutionContext.cpp:396 > + publicURL.setProtocol("x-raw-media"); http://groups.google.com/a/chromium.org/group/chromium-reviews/browse_thread/thread/c31593de00fba166?hide_quotes=no&fwc=2&pli=1 is the only reference to x-raw-media I can find on the Internet. We shouldn't be making up URL schemes, especially ones that are visible externally.
Created attachment 103759 [details] Patch
Comment on attachment 103488 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103488&action=review >> LayoutTests/fast/files/create-blob-url-crash-expected.txt:2 >> +PASS: Type error > > IMHO, we shouldn't change this behavior. We probably need to fix the code generator to generate correct code. Would it be OK to change this for now, considering that a bug (66045) is created for changing the code generators? >> Source/WebCore/ChangeLog:7 >> + > > This ChangeLog doesn't explain why we're making this change or why this patch doesn't contain tests. Is there some reason we can't test this patch? Added bug and short explanation. >> Source/WebCore/dom/ScriptExecutionContext.cpp:396 >> + publicURL.setProtocol("x-raw-media"); > > http://groups.google.com/a/chromium.org/group/chromium-reviews/browse_thread/thread/c31593de00fba166?hide_quotes=no&fwc=2&pli=1 is the only reference to x-raw-media I can find on the Internet. We shouldn't be making up URL schemes, especially ones that are visible externally. Fixed this to be at least standards compliant.
(In reply to comment #11) > (From update of attachment 103488 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=103488&action=review > > >> LayoutTests/fast/files/create-blob-url-crash-expected.txt:2 > >> +PASS: Type error > > > > IMHO, we shouldn't change this behavior. We probably need to fix the code generator to generate correct code. > > Would it be OK to change this for now, considering that a bug (66045) is created for changing the code generators? Sure. This is a minor issue. > >> Source/WebCore/ChangeLog:7 > >> + > > > > This ChangeLog doesn't explain why we're making this change or why this patch doesn't contain tests. Is there some reason we can't test this patch? > > Added bug and short explanation. Great. > >> Source/WebCore/dom/ScriptExecutionContext.cpp:396 > >> + publicURL.setProtocol("x-raw-media"); > > > > http://groups.google.com/a/chromium.org/group/chromium-reviews/browse_thread/thread/c31593de00fba166?hide_quotes=no&fwc=2&pli=1 is the only reference to x-raw-media I can find on the Internet. We shouldn't be making up URL schemes, especially ones that are visible externally. > > Fixed this to be at least standards compliant. Ok.
Comment on attachment 103759 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103759&action=review R- for the static initializer. I don't know how I feel about the fragment identifier hack. It doesn't seem like the right solution to the problem. On the one hand, I don't want to block you from making progress here. On the other hand, I don't want to expose these internal hacks to the platform. :( > Source/WebCore/dom/ScriptExecutionContext.cpp:390 > +static const String kMediaStreamFragment = "mediastream"; This is a static initializer, which is banned in WebKit. Instead, use DEFINE_STATIC_LOCAL in a function. > Source/WebCore/dom/ScriptExecutionContext.cpp:399 > + // This makes it faster to detect that this URL points to a MediaStream. > + publicURL.setFragmentIdentifier(kMediaStreamFragment); This is a bit of a hack. I saw that you started a standards thread about creating a new scheme. On the one hand, I don't want to prevent you from making progress, but on the other hand I'm worried that this sort of thing is likely to stick around. > Source/WebCore/platform/MediaStreamRegistry.cpp:39 > + DEFINE_STATIC_LOCAL(MediaStreamRegistry, instance, ()); Here's an example of how to avoid static initializers.
Comment on attachment 103759 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103759&action=review >> Source/WebCore/dom/ScriptExecutionContext.cpp:390 >> +static const String kMediaStreamFragment = "mediastream"; > > This is a static initializer, which is banned in WebKit. Instead, use DEFINE_STATIC_LOCAL in a function. Removed. >> Source/WebCore/dom/ScriptExecutionContext.cpp:399 >> + publicURL.setFragmentIdentifier(kMediaStreamFragment); > > This is a bit of a hack. I saw that you started a standards thread about creating a new scheme. On the one hand, I don't want to prevent you from making progress, but on the other hand I'm worried that this sort of thing is likely to stick around. Hack removed, we just have to do an extra roundtrip chrome->webkit to ask if the blob url is a mediastream or blob.
Created attachment 104653 [details] Patch
Comment on attachment 104653 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104653&action=review > Source/WebCore/dom/ScriptExecutionContext.cpp:390 > +static const String kMediaStreamFragment = "mediastream"; This is still a static initializer. These are forbidden in WebKit. > Source/WebCore/dom/ScriptExecutionContext.cpp:399 > + // This makes it faster to detect that this URL points to a MediaStream. > + publicURL.setFragmentIdentifier(kMediaStreamFragment); I though you removed this hack? /me is confused.
Comment on attachment 104653 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104653&action=review >> Source/WebCore/dom/ScriptExecutionContext.cpp:399 >> + publicURL.setFragmentIdentifier(kMediaStreamFragment); > > I though you removed this hack? /me is confused. So am I...
Created attachment 104694 [details] Patch
Comment on attachment 104694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104694&action=review > Source/WebCore/dom/ScriptExecutionContext.cpp:400 > + mediaStreamRegistry().registerMediaStreamURL(publicURL, stream); > + m_publicStreamURLs.add(publicURL.string()); It's slightly unfortunate that we need to register this URL in two places, but I don't see how to avoid that. > Source/WebCore/platform/MediaStreamRegistry.h:42 > + PassRefPtr<MediaStream> mediaStream(const KURL&) const; Any particular reason why this returns a PassRefPtr rather than a raw pointer? We're not transferring ownership here. > Source/WebCore/platform/MediaStreamRegistry.h:45 > + // Currently assuming that the URLs keep alive their associated Streams. How is that enforced? (Technically comments should be complete sentences.)
Comment on attachment 104694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104694&action=review >> Source/WebCore/dom/ScriptExecutionContext.cpp:400 >> + m_publicStreamURLs.add(publicURL.string()); > > It's slightly unfortunate that we need to register this URL in two places, but I don't see how to avoid that. No, neither can I. If we some day can have our own protocol I'll look at this again. >> Source/WebCore/platform/MediaStreamRegistry.h:42 >> + PassRefPtr<MediaStream> mediaStream(const KURL&) const; > > Any particular reason why this returns a PassRefPtr rather than a raw pointer? We're not transferring ownership here. Fixed. Sorry... >> Source/WebCore/platform/MediaStreamRegistry.h:45 >> + // Currently assuming that the URLs keep alive their associated Streams. > > How is that enforced? (Technically comments should be complete sentences.) Removed comment since it wasn't up to date.
Created attachment 104996 [details] Patch
Thanks for iterating on this patch.
Comment on attachment 104996 [details] Patch Clearing flags on attachment: 104996 Committed r93713: <http://trac.webkit.org/changeset/93713>
All reviewed patches have been landed. Closing bug.
(In reply to comment #5) > (From update of attachment 103375 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=103375&action=review > > >> LayoutTests/fast/files/create-blob-url-crash-expected.txt:2 > >> +PASS: Type error > > > > Why is this exception changing type? I suspect we need to change the code generator to check the number of arguments before it checks the type of the arguments. > > No idea as to why the code the code generator acts differently now, but I have verified that the JS bindings throw a type error as well. Actually they don't. (At least when this define is turned off.) So this creates a difference in out expected results which isn't good. Sam fixed the expectation for JS here: http://trac.webkit.org/changeset/94023 (and now we'll need a special expectation for this test -- which is bad).