Bug 65925

Summary: MediaStream API: add createObjectURL functionality
Product: WebKit Reporter: Tommy Widenflycht <tommyw>
Component: DOMAssignee: Tommy Widenflycht <tommyw>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, donggwan.kim, leandrogracia, levin, tonyg, webkit.review.bot, wjjeon
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 66045    
Bug Blocks: 56459    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Tommy Widenflycht 2011-08-09 10:03:30 PDT
Adding window.URL.createObjectURL(MediaStream) with associated supporting code.
Comment 1 Tommy Widenflycht 2011-08-09 10:08:11 PDT
Created attachment 103367 [details]
Patch
Comment 2 WebKit Review Bot 2011-08-09 10:54:06 PDT
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
Comment 3 Tommy Widenflycht 2011-08-09 11:14:44 PDT
Created attachment 103375 [details]
Patch
Comment 4 Adam Barth 2011-08-09 11:30:50 PDT
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 5 Tommy Widenflycht 2011-08-10 05:31:56 PDT
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).
Comment 6 Tommy Widenflycht 2011-08-10 05:33:56 PDT
Created attachment 103471 [details]
Patch
Comment 7 Tommy Widenflycht 2011-08-10 05:53:55 PDT
Created attachment 103476 [details]
Patch
Comment 8 Tommy Widenflycht 2011-08-10 07:32:09 PDT
Created attachment 103488 [details]
Patch
Comment 9 Adam Barth 2011-08-10 10:06:42 PDT
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.
Comment 10 Tommy Widenflycht 2011-08-12 04:35:10 PDT
Created attachment 103759 [details]
Patch
Comment 11 Tommy Widenflycht 2011-08-12 04:38:05 PDT
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.
Comment 12 Adam Barth 2011-08-12 11:45:47 PDT
(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 13 Adam Barth 2011-08-12 11:59:11 PDT
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 14 Tommy Widenflycht 2011-08-22 02:13:27 PDT
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.
Comment 15 Tommy Widenflycht 2011-08-22 02:15:28 PDT
Created attachment 104653 [details]
Patch
Comment 16 Adam Barth 2011-08-22 10:30:42 PDT
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 17 Tommy Widenflycht 2011-08-22 10:37:07 PDT
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...
Comment 18 Tommy Widenflycht 2011-08-22 10:41:44 PDT
Created attachment 104694 [details]
Patch
Comment 19 Adam Barth 2011-08-22 10:48:42 PDT
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 20 Tommy Widenflycht 2011-08-24 07:23:43 PDT
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.
Comment 21 Tommy Widenflycht 2011-08-24 07:30:12 PDT
Created attachment 104996 [details]
Patch
Comment 22 Adam Barth 2011-08-24 10:14:51 PDT
Thanks for iterating on this patch.
Comment 23 WebKit Review Bot 2011-08-24 11:07:57 PDT
Comment on attachment 104996 [details]
Patch

Clearing flags on attachment: 104996

Committed r93713: <http://trac.webkit.org/changeset/93713>
Comment 24 WebKit Review Bot 2011-08-24 11:08:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 David Levin 2011-08-29 16:28:06 PDT
(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).