Bug 65925 – MediaStream API: add createObjectURL functionality

• Component: DOM
• Status: RESOLVED FIXED

[Tommy Widenflycht]

Adding window.URL.createObjectURL(MediaStream) with associated supporting code.

[Tommy Widenflycht]

Created attachment 103367 [details]
Patch

[WebKit Review Bot]

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

[Tommy Widenflycht]

Created attachment 103375 [details]
Patch

[Adam Barth]

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.

[Tommy Widenflycht]

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

[Tommy Widenflycht]

Created attachment 103471 [details]
Patch

[Tommy Widenflycht]

Created attachment 103476 [details]
Patch

[Tommy Widenflycht]

Created attachment 103488 [details]
Patch

[Adam Barth]

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.

[Tommy Widenflycht]

Created attachment 103759 [details]
Patch

[Tommy Widenflycht]

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.

[Adam Barth]

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

[Adam Barth]

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.

[Tommy Widenflycht]

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.

[Tommy Widenflycht]

Created attachment 104653 [details]
Patch

[Adam Barth]

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.

[Tommy Widenflycht]

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

[Tommy Widenflycht]

Created attachment 104694 [details]
Patch

[Adam Barth]

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

[Tommy Widenflycht]

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.

[Tommy Widenflycht]

Created attachment 104996 [details]
Patch

[Adam Barth]

Thanks for iterating on this patch.

[WebKit Review Bot]

Comment on attachment 104996 [details]
Patch

Clearing flags on attachment: 104996

Committed r93713: <http://trac.webkit.org/changeset/93713>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[David Levin]

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