[Webkit-unassigned] [Bug 56586] Media Stream API patch 1: adding the getUserMedia method and the basic skeleton

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=56586





--- Comment #12 from Leandro Graciá Gil <leandrogracia at chromium.org>  2011-03-22 11:55:04 PST ---
(From update of attachment 86198)
View in context: https://bugs.webkit.org/attachment.cgi?id=86198&action=review

>> Source/WebCore/page/NavigatorUserMediaError.idl:33
>> +    // FIXME: delete this interface and implement a custom binding since spec defines it as a NoInterfaceObject.
> 
> Any reason not to do this now?

Fixed for all callback objects. Since the spec is still changing and being discussed we're delaying the removal of the idl file for NavigatorUserMediaError.

>> Source/WebCore/page/NavigatorUserMediaErrorCallback.cpp:46
>> +// NavigatorUserMediaSuccessCallback and BlobCallback.
> 
> Again, why not do this now?

Done. Check CallbackTask.h file in the new patch.

>> Source/WebCore/page/Page.cpp:858
>> +        m_streamController.set(new StreamController(m_streamClient, mainFrame()->document()));
> 
> Do you plan to add a runtime flag? Other controller/client features only instantiate the controller when the runtime flag is set, as the controller is otherwsise not used. Make sure that the client is still signalled to shut down, if required, in this case though.
> 
> Also, why doe we use the main frame's document? How does this work in the case of iframes?

Yes, we plan to add a runtime flag in chromium. The new patch also introduces the EnabledAtRuntime flag to the corresponding idl files.

Fixed the problems with the frame, the security origins and replying using the same execution context that made the call.

>> Source/WebCore/page/Page.h:344
>> +        OwnPtr<StreamController> m_streamController;
> 
> I understand that the spec refers to this as just 'Stream', but the name seems a little vague. Could you use a more descriptive name, or at least add a comment about what kind of stream it is?

This is not a Stream but the class controlling them and acting as an interface to the client. Streams are introduced in bug 56666. In any case, I've added a comment in the StreamController.h and Stream.h (next patch) files to make clear that this file is about the Media Stream API.

>> Source/WebCore/page/StreamListener.h:41
>> +// be implemented by both the StreamController and the StreamClientMock.
> 
> I don't understand this. Can you explain why the mock client would implement this interface?

Fixed. StreamListener removed, as the StreamController is now used directly.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.