Bug 70897

Hmm, I am not 100% certain what you mean here. So this would be optional? In Chromium we definitely doesn't want a separate user content dialog since we will combine getting user consent with audio/video device selection. (In reply to comment #0) > The client should only be used for user consent when there are platform interfaces to do the low-level platform specific calls.

(In reply to comment #1) > In Chromium we definitely doesn't want a separate user content dialog since we will combine getting user consent with audio/video device selection. That's how we do it as well. Selecting a device implies giving consent.

Created attachment 113144 [details] Proposed patch

Comment on attachment 113144 [details] Proposed patch Attachment 113144 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10250177

Comment on attachment 113144 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=113144&action=review > Source/WebCore/WebCore.gypi:2907 > + 'mediastream/UserMediaClient.h", " -> '

Comment on attachment 113144 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=113144&action=review > Source/WebCore/mediastream/UserMediaController.cpp:36 > + Need #include "DOMWindow.h" as well.

Comment on attachment 113144 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=113144&action=review > Source/WebCore/mediastream/UserMediaController.h:50 > +class UserMediaQueryClient { One class per file > Source/WebCore/mediastream/UserMediaController.h:58 > +class UserMediaRequest : public RefCounted<UserMediaRequest> { One class per file

Could you remove the user consent query functionality from this patch? It complicates the patch and nobody (at least that I know of) is going to use it.

Comment on attachment 113144 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=113144&action=review > Source/WebCore/mediastream/UserMediaClient.h:36 > +#include "PlatformString.h" Please include <wtf/WTFString.h>, which is the new name for PlatformString.h. > Source/WebCore/mediastream/UserMediaClient.h:47 > + virtual void requestUserMedia(const UserMediaRequest*) = 0; > + virtual void cancelUserMediaRequest(const UserMediaRequest*) = 0; > + We don't really use const pointers in parameters. They're kind of meaningless. > Source/WebCore/mediastream/UserMediaController.cpp:46 > +PassRefPtr<UserMediaRequest> UserMediaRequest::create(const String& options, PassRefPtr<NavigatorUserMediaSuccessCallback> successCallback, PassRefPtr<NavigatorUserMediaErrorCallback> errorCallback, Frame* frame) Should Frame be first? Usually we pass context objects like Frame as the first parameter. > Source/WebCore/mediastream/UserMediaController.cpp:63 > + , m_queryClient(0) m_queryClient ? Is there a better name we could use here? > Source/WebCore/mediastream/UserMediaController.cpp:68 > +void UserMediaRequest::parseOptions(const String& options) Does this need to be case insensitive. > Source/WebCore/mediastream/UserMediaController.cpp:73 > + for (size_t i = 0; i < optionsList.size(); i++) { ++i > Source/WebCore/mediastream/UserMediaController.cpp:98 > + m_client->userMediaControllerDestroyed(); userMediaControllerDestroyed -> willDestroyUserMedia probably. > Source/WebCore/mediastream/UserMediaController.cpp:108 > + request->setQueryClient(this); What is a query client? > Source/WebCore/mediastream/UserMediaController.cpp:134 > + DOMWindow* domWindow = request->frame()->existingDOMWindow(); > + if (!domWindow) > + return; This seems wrong. The request should hold the document as context, not the frame. This design runs the risk of delivering the response to the wrong document! > Source/WebCore/mediastream/UserMediaController.cpp:145 > + size_t i = m_requests.find(request); > + if (i != notFound) > + m_requests.remove(i); Should we assert that we're not in the middle of canceling all these requests? How it be that the request isn't found? > Source/WebCore/mediastream/UserMediaController.h:86 > + bool m_audio; > + bool m_video; > + > + bool m_cameraPreferenceUser; > + bool m_cameraPreferenceEnvironment; > + > + RefPtr<NavigatorUserMediaSuccessCallback> m_successCallback; > + RefPtr<NavigatorUserMediaErrorCallback> m_errorCallback; > + > + Frame* m_frame; > + UserMediaQueryClient* m_queryClient; These should all be private. > Source/WebCore/page/Frame.cpp:231 > #if ENABLE(MEDIA_STREAM) > - if (m_mediaStreamFrameController) > - m_mediaStreamFrameController->disconnectFrame(); > + if (m_page && m_page->userMediaController()) > + m_page->userMediaController()->cancelAllUserMediaRequestsFromFrame(this); > #endif userMediaController should be a FrameDestructionObserver rather than being explicitly understood by Frame. > Source/WebCore/page/Navigator.cpp:90 > +#if ENABLE(MEDIA_STREAM) > + if (m_frame && m_frame->page()) > + m_frame->page()->userMediaController()->cancelAllUserMediaRequestsFromFrame(m_frame); > +#endif There way too many of call points. This is likely to be very buggy. We need to figure out where the right place to cancel these requests is and do it exactly there. > Source/WebCore/page/Page.cpp:144 > +#if ENABLE(MEDIA_STREAM) > + , m_userMediaController(adoptPtr(new UserMediaController(this, pageClients.userMediaClient))) > +#endif I'm not 100% sold on a Page-level controller. Are there alternative designs?

Created attachment 114178 [details] Updated patch (In reply to comment #5) > (From update of attachment 113144 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113144&action=review > > > Source/WebCore/WebCore.gypi:2907 > > + 'mediastream/UserMediaClient.h", > > " -> ' Fixed. (In reply to comment #6) > (From update of attachment 113144 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113144&action=review > > > Source/WebCore/mediastream/UserMediaController.cpp:36 > > + > > Need #include "DOMWindow.h" as well. Fixed. (In reply to comment #7) > (From update of attachment 113144 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113144&action=review > > > Source/WebCore/mediastream/UserMediaController.h:50 > > +class UserMediaQueryClient { > > One class per file Class removed. > > > Source/WebCore/mediastream/UserMediaController.h:58 > > +class UserMediaRequest : public RefCounted<UserMediaRequest> { > > One class per file Fixed. (In reply to comment #8) > Could you remove the user consent query functionality from this patch? It complicates the patch and nobody (at least that I know of) is going to use it. The query functionality in this patch is used to ask the platform about available media stream sources. The query results can then be presented to the user in the consent UI. (In reply to comment #9) > (From update of attachment 113144 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113144&action=review > > > Source/WebCore/mediastream/UserMediaClient.h:36 > > +#include "PlatformString.h" > > Please include <wtf/WTFString.h>, which is the new name for PlatformString.h. Fixed. > > Source/WebCore/mediastream/UserMediaClient.h:47 > > + virtual void requestUserMedia(const UserMediaRequest*) = 0; > > + virtual void cancelUserMediaRequest(const UserMediaRequest*) = 0; > > + > > We don't really use const pointers in parameters. They're kind of meaningless. Fixed. > > Source/WebCore/mediastream/UserMediaController.cpp:46 > > +PassRefPtr<UserMediaRequest> UserMediaRequest::create(const String& options, PassRefPtr<NavigatorUserMediaSuccessCallback> successCallback, PassRefPtr<NavigatorUserMediaErrorCallback> errorCallback, Frame* frame) > > Should Frame be first? Usually we pass context objects like Frame as the first parameter. Fixed. > > Source/WebCore/mediastream/UserMediaController.cpp:63 > > + , m_queryClient(0) > > m_queryClient ? Is there a better name we could use here? I've removed the UserMediaQueryClient interface so this member has been replaced. > > Source/WebCore/mediastream/UserMediaController.cpp:68 > > +void UserMediaRequest::parseOptions(const String& options) > > Does this need to be case insensitive. No, it should be case sensitive. > > Source/WebCore/mediastream/UserMediaController.cpp:73 > > + for (size_t i = 0; i < optionsList.size(); i++) { > > ++i ? > > Source/WebCore/mediastream/UserMediaController.cpp:98 > > + m_client->userMediaControllerDestroyed(); > > userMediaControllerDestroyed -> willDestroyUserMedia probably. I aligned this name with other clients (e.g. deviceMotionControllerDestroyed, dragControllerDestroyed). > > Source/WebCore/mediastream/UserMediaController.cpp:108 > > + request->setQueryClient(this); > > What is a query client? It was the client of the platform user media probing functionality; it has been removed in the updated patch. > > Source/WebCore/mediastream/UserMediaController.cpp:134 > > + DOMWindow* domWindow = request->frame()->existingDOMWindow(); > > + if (!domWindow) > > + return; > > This seems wrong. The request should hold the document as context, not the frame. This design runs the risk of delivering the response to the wrong document! True. Added a check to verify that the context is the same as when the request was made. > > Source/WebCore/mediastream/UserMediaController.cpp:145 > > + size_t i = m_requests.find(request); > > + if (i != notFound) > > + m_requests.remove(i); > > Should we assert that we're not in the middle of canceling all these requests? How it be that the request isn't found? Fixed. > > Source/WebCore/mediastream/UserMediaController.h:86 > > + bool m_audio; > > + bool m_video; > > + > > + bool m_cameraPreferenceUser; > > + bool m_cameraPreferenceEnvironment; > > + > > + RefPtr<NavigatorUserMediaSuccessCallback> m_successCallback; > > + RefPtr<NavigatorUserMediaErrorCallback> m_errorCallback; > > + > > + Frame* m_frame; > > + UserMediaQueryClient* m_queryClient; > > These should all be private. Fixed (this used to be a struct with public members and they were accidentally left public) > > Source/WebCore/page/Frame.cpp:231 > > #if ENABLE(MEDIA_STREAM) > > - if (m_mediaStreamFrameController) > > - m_mediaStreamFrameController->disconnectFrame(); > > + if (m_page && m_page->userMediaController()) > > + m_page->userMediaController()->cancelAllUserMediaRequestsFromFrame(this); > > #endif > > userMediaController should be a FrameDestructionObserver rather than being explicitly understood by Frame. Made UserMediaRequest a FrameDestructionObserver. > > Source/WebCore/page/Navigator.cpp:90 > > +#if ENABLE(MEDIA_STREAM) > > + if (m_frame && m_frame->page()) > > + m_frame->page()->userMediaController()->cancelAllUserMediaRequestsFromFrame(m_frame); > > +#endif > > There way too many of call points. This is likely to be very buggy. We need to figure out where the right place to cancel these requests is and do it exactly there. Handeled via FrameDescrutionObserver now. > > Source/WebCore/page/Page.cpp:144 > > +#if ENABLE(MEDIA_STREAM) > > + , m_userMediaController(adoptPtr(new UserMediaController(this, pageClients.userMediaClient))) > > +#endif > > I'm not 100% sold on a Page-level controller. Are there alternative designs? We could put more logic into the UserMediaRequest and skip the controller. However, it may be convenient to have the controller when we add stuff like muting of media sources from the browser chrome and similar. Btw, I seems that the bindings for getUserMedia() can be generated, except that the callbacks don't become FunctionOnly.

Comment on attachment 114178 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=114178&action=review Below are a few nits. > Source/WebCore/mediastream/UserMediaController.cpp:90 > + DOMWindow* domWindow = request->frame()->existingDOMWindow(); > + if (!domWindow) > + return; > + > + ScriptExecutionContext* context = domWindow->scriptExecutionContext(); > + if (!context || context != request->scriptExecutionContext()) > + return; You should just get the ScriptExecutionContext from the frame without involving the DOMWindow: ScriptExecutionContext* context = request->frame()->document(); > Source/WebCore/mediastream/UserMediaRequest.cpp:57 > + , m_scriptExecutionContext(frame->existingDOMWindow()->scriptExecutionContext()) Please don't go through the DOMWindow. frame->document() should work just find here. > Source/WebCore/mediastream/UserMediaRequest.cpp:78 > + for (size_t i = 0; i < optionsList.size(); i++) { ++i > Source/WebCore/mediastream/UserMediaRequest.cpp:80 > + optionsList[i].split(" ", subOptionsList); Are you sure this is right? Usually these parsers use isHTMLSpace. Can you put a link the spec in the code? > Source/WebCore/mediastream/UserMediaRequest.cpp:82 > + if (subOptionsList[0] == "audio") This is really supposed to be case sensitive? > Source/WebCore/mediastream/UserMediaRequest.cpp:99 > + m_frame = 0; Do we need to zero out m_controller too? > Source/WebCore/mediastream/UserMediaRequest.h:82 > + ScriptExecutionContext* m_scriptExecutionContext; Does this need to be a RefPtr?

> > Could you remove the user consent query functionality from this patch? It complicates the patch and nobody (at least that I know of) is going to use it. > > The query functionality in this patch is used to ask the platform about available media stream sources. The query results can then be presented to the user in the consent UI. Tommy, are you happy with this response?

Comment on attachment 114178 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=114178&action=review > Source/WebCore/mediastream/UserMediaController.cpp:53 > + m_client->userMediaControllerDestroyed(); At this point have all outstanding requests been cancelled? Would be best, I think, to do so. >> Source/WebCore/mediastream/UserMediaRequest.cpp:78 >> + for (size_t i = 0; i < optionsList.size(); i++) { > > ++i FYI: This means that you should change your i++ to ++i. A prefix operator is generally much better than a postfix one.

(In reply to comment #12) > > > Could you remove the user consent query functionality from this patch? It complicates the patch and nobody (at least that I know of) is going to use it. > > > > The query functionality in this patch is used to ask the platform about available media stream sources. The query results can then be presented to the user in the consent UI. > > Tommy, are you happy with this response? It's fine by me. I have nothing against a query interface, it just feels weird that the actual creation of the streams goes through a PageClient but the approval is supposed to go through the platform interface. My main objection was that it for this initial patch it might be better to not have a query interface for complexity reasons. Approvals/declines can just as well be handled through the userMediaRequestSucceeded/userMediaRequestFailed callbacks in the UserMediaController.

Comment on attachment 114178 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=114178&action=review > Source/WebCore/mediastream/UserMediaController.cpp:92 > + RefPtr<LocalMediaStream> stream = LocalMediaStream::create(context, sources); Would it be possible for the label to be provided by the embedder instead of the LocalMediaStream class? This would help us a lot!

> It's fine by me. I have nothing against a query interface, it just feels weird that the actual creation of the streams goes through a PageClient but the approval is supposed to go through the platform interface. Oh, I didn't understand that from the patch. That does seem backwards. Approvals should go through the Client and creation should go through the Platform.

I think a design doc, flow charts or similar would be immensely helpful here and now.

(In reply to comment #16) > > It's fine by me. I have nothing against a query interface, it just feels weird that the actual creation of the streams goes through a PageClient but the approval is supposed to go through the platform interface. > > Oh, I didn't understand that from the patch. That does seem backwards. Approvals should go through the Client and creation should go through the Platform. That's not how it works either. This patch leaves the approvals/declines to the implementor of the client via userMediaRequestSucceeded/userMediaRequestFailed as Tommy describes above. The query functionality is for "asking" the platform for a list of user media sources, which then can be presented in the user consent UI. This is necessary for us to access the GStreamer device probing functionality in the platform. If you don't need it, you don't have to use it.

1) a JS function calls GetUserMedia which forwards the call to UserMediaController 2) The UserMediaController enumerates the available audio/video devices through the Platform interface. The devices are at this stage unopened. 3) The UserMediaController then sends the list for selection and approval from the user through the Client interface. 4) The user selects say a camera and a audio input device and thus approves the use. Have I gotten it right this time? I have been confused by the wording Query which for me sounds like "asking the user" and not "enumerating all devices". In that case the Client implementation must open the devices, otherwise we can't handle problems with device availability not show a self preview etc etc. This means that the Client implementation is going to to the bulk of the functionality and not just simply doing a policy decision. I personally is OK with this but I am bringing this up so that it won't pop up later in the process (been there, done that).

Created attachment 114476 [details] Patch 3 (In reply to comment #11) > (From update of attachment 114178 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=114178&action=review > > Below are a few nits. > > > Source/WebCore/mediastream/UserMediaController.cpp:90 > > + DOMWindow* domWindow = request->frame()->existingDOMWindow(); > > + if (!domWindow) > > + return; > > + > > + ScriptExecutionContext* context = domWindow->scriptExecutionContext(); > > + if (!context || context != request->scriptExecutionContext()) > > + return; > > You should just get the ScriptExecutionContext from the frame without involving the DOMWindow: > > ScriptExecutionContext* context = request->frame()->document(); Fixed. > > Source/WebCore/mediastream/UserMediaRequest.cpp:57 > > + , m_scriptExecutionContext(frame->existingDOMWindow()->scriptExecutionContext()) > > Please don't go through the DOMWindow. frame->document() should work just find here. Fixed. > > Source/WebCore/mediastream/UserMediaRequest.cpp:78 > > + for (size_t i = 0; i < optionsList.size(); i++) { > > ++i Fixed (Perhaps this should be added to the style guidelines?) > > Source/WebCore/mediastream/UserMediaRequest.cpp:80 > > + optionsList[i].split(" ", subOptionsList); > > Are you sure this is right? Usually these parsers use isHTMLSpace. Can you put a link the spec in the code? You're right, the split was a bit naive. I use SpaceSplitString now. > > Source/WebCore/mediastream/UserMediaRequest.cpp:82 > > + if (subOptionsList[0] == "audio") > > This is really supposed to be case sensitive? Yes. "2. If the first token in list of suboptions is a case-sensitive match for the string "audio", let audio be true." > > Source/WebCore/mediastream/UserMediaRequest.cpp:99 > > + m_frame = 0; > > Do we need to zero out m_controller too? The controller is zeroed out in the call to cancelUserMediaRequset() on line before. > > Source/WebCore/mediastream/UserMediaRequest.h:82 > > + ScriptExecutionContext* m_scriptExecutionContext; > > Does this need to be a RefPtr? We only use this member to compare the pointer value with the context at the time a request succeeds or fails. (In reply to comment #13) > (From update of attachment 114178 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=114178&action=review > > > Source/WebCore/mediastream/UserMediaController.cpp:53 > > + m_client->userMediaControllerDestroyed(); > > At this point have all outstanding requests been cancelled? Would be best, I think, to do so. They should be canceled at this point. > >> Source/WebCore/mediastream/UserMediaRequest.cpp:78 > >> + for (size_t i = 0; i < optionsList.size(); i++) { > > > > ++i > > FYI: This means that you should change your i++ to ++i. A prefix operator is generally much better than a postfix one. I got that, but I was just wondering what the motivation was (it's a pure style issue in this particular case). I haven't got this comment before and there's nothing about it in the style guidelines (perhaps it should be added?). It's anyhow fixed now. (In reply to comment #19) > 1) a JS function calls GetUserMedia which forwards the call to UserMediaController > 2) The UserMediaController enumerates the available audio/video devices through the Platform interface. The devices are at this stage unopened. > 3) The UserMediaController then sends the list for selection and approval from the user through the Client interface. > 4) The user selects say a camera and a audio input device and thus approves the use. > > Have I gotten it right this time? I have been confused by the wording Query which for me sounds like "asking the user" and not "enumerating all devices". Yes, that's about it.

> > >> Source/WebCore/mediastream/UserMediaRequest.cpp:78 > > >> + for (size_t i = 0; i < optionsList.size(); i++) { > > > > > > ++i > > > > FYI: This means that you should change your i++ to ++i. A prefix operator is generally much better than a postfix one. > > I got that, but I was just wondering what the motivation was (it's a pure style issue in this particular case). I haven't got this comment before and there's nothing about it in the style guidelines (perhaps it should be added?). It's anyhow fixed now. For an int etc it doesn't matter but if you have an object (like an iterator) the compiler has to clone the object if you use a postfix operator. Best all around to avoid postfix unless you really need it.

Comment on attachment 114476 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=114476&action=review > Source/WebCore/mediastream/UserMediaController.cpp:94 > +void UserMediaController::userMediaRequestSucceeded(UserMediaRequest* request, const MediaStreamSourceVector& sources) How about letting the embedder provide a label as well? Instead of letting LocalMediaStream create one. This would help at least the chromium port.

Comment on attachment 114476 [details] Patch 3 Attachment 114476 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10395309

(In reply to comment #22) > (From update of attachment 114476 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=114476&action=review > > > Source/WebCore/mediastream/UserMediaController.cpp:94 > > +void UserMediaController::userMediaRequestSucceeded(UserMediaRequest* request, const MediaStreamSourceVector& sources) > > How about letting the embedder provide a label as well? Instead of letting LocalMediaStream create one. This would help at least the chromium port. I didn't think you wanted this anymore since MediaStream instances cannot be identified by the label. Why do you need it? (The label is a pure API related thing).

> > > > How about letting the embedder provide a label as well? Instead of letting LocalMediaStream create one. This would help at least the chromium port. > > I didn't think you wanted this anymore since MediaStream instances cannot be identified by the label. Why do you need it? (The label is a pure API related thing). After discussions we are OK with the current patch, and we'll adjust the code accordingly. It was a request from the native library.

Comment on attachment 114476 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=114476&action=review > Source/WebCore/mediastream/UserMediaController.cpp:82 > + ASSERT(m_requests.contains(request)); This function needs to be part of UserMediaController to use m_requests > Source/WebCore/mediastream/UserMediaController.cpp:87 > + ScriptExecutionContext* context = request->frame()->document(); Need to #include "Document.h" for this to compile. > Source/WebCore/mediastream/UserMediaRequest.cpp:58 > + , m_scriptExecutionContext(frame->document()) Need to #include "Document.h" for this to compile.

> For an int etc it doesn't matter but if you have an object (like an iterator) the compiler has to clone the object if you use a postfix operator. Best all around to avoid postfix unless you really need it. It's totally not a big deal. I thought it was in the style guide, but I might be wrong.

Created attachment 114706 [details] Patch 4 (In reply to comment #26) > (From update of attachment 114476 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=114476&action=review > > > Source/WebCore/mediastream/UserMediaController.cpp:82 > > + ASSERT(m_requests.contains(request)); > > This function needs to be part of UserMediaController to use m_requests Fixed. > > Source/WebCore/mediastream/UserMediaController.cpp:87 > > + ScriptExecutionContext* context = request->frame()->document(); > > Need to #include "Document.h" for this to compile. Fixed. > > Source/WebCore/mediastream/UserMediaRequest.cpp:58 > > + , m_scriptExecutionContext(frame->document()) > > Need to #include "Document.h" for this to compile. Fixed.

Comment on attachment 114706 [details] Patch 4 View in context: https://bugs.webkit.org/attachment.cgi?id=114706&action=review > Source/WebCore/mediastream/UserMediaClient.h:43 > + virtual void userMediaControllerDestroyed() = 0; On another bug, fishd had a question about this name because userMediaController is something of a WebCore-concept. It's probably better to have a more semantic name here, but that's something we can address in a follow-up patch. > Source/WebCore/mediastream/UserMediaController.cpp:64 > + m_requests.append(request); m_requests is something like m_outstandingRequests ? > Source/WebCore/mediastream/UserMediaController.cpp:88 > + ScriptExecutionContext* context = request->frame()->document(); The request should hold the document explicitly rather than via the Frame. Generally, using the Frame as the context object is dangerous because the frame persists across navigations. > Source/WebCore/mediastream/UserMediaController.cpp:89 > + if (!context || context != request->scriptExecutionContext()) I see. You check explicitly here.... This is somewhat of an error prone design. > Source/WebCore/mediastream/UserMediaController.cpp:101 > + request->successCallback()->handleEvent(stream.get()); I assume this can run arbitrary script. How do we know that |this| hasn't been destroyed? I'd remove request from m_requests before calling the success callback to avoid getting into inconsistent states. > Source/WebCore/mediastream/UserMediaController.cpp:115 > + request->errorCallback()->handleEvent(error.get()); Same comment there. > Source/WebCore/mediastream/UserMediaController.h:46 > +class UserMediaController { I don't get the point of this class. We do all this work to track the outstanding requests in m_requests, but then we never do anything with that collection. I was expecting some kind of "cancel all" operation when something got torn down, but I don't see that anywhere. > Source/WebCore/mediastream/UserMediaRequest.h:83 > + Frame* m_frame; > + ScriptExecutionContext* m_scriptExecutionContext; > + UserMediaController* m_controller; It's really strange to keep all of these context objects. Really, this object should hold only a ScriptExecutionContext and should be a ContextDestructionObserver. Moreover, it seems like UserMediaController isn't needed at all. > Source/WebCore/page/Navigator.cpp:302 > + page->userMediaController()->requestUserMedia(request.release()); IMHO, this should just talk directly to the UserMediaClient.

I see that previous iterations of this patch had a "cancel all" operation, but the most recent one doesn't seem to have that.

> > I'm not 100% sold on a Page-level controller. Are there alternative designs? > > We could put more logic into the UserMediaRequest and skip the controller. However, it may be convenient to have the controller when we add stuff like muting of media sources from the browser chrome and similar. What's you opinion about skipping the controller as described above (from comment #10)? An other example of a client initiated action is if the browser wants to probe for devices via the platform on startup.

> What's you opinion about skipping the controller as described above (from comment #10)? An other example of a client initiated action is if the browser wants to probe for devices via the platform on startup. Let's skip the controller for now. We can always add it later if we need it.

Created attachment 115412 [details] Patch 5 Removed the controller.

Comment on attachment 115412 [details] Patch 5 View in context: https://bugs.webkit.org/attachment.cgi?id=115412&action=review I think we should land this patch so we can move forward. This patch is generally very good. I have one memory-safety concern abou the lifetime of the UserMediaClient, which I've noted below, but we can address that issue in a follow-up patch. Thanks again for iterating on this patch. > Source/WebCore/mediastream/UserMediaRequest.cpp:105 > + if (m_client) { > + m_client->cancelUserMediaRequest(this); > + m_client = 0; > + } What is the lifetime of the UserMediaClient? In particular, can a document out-live the client?

Comment on attachment 115412 [details] Patch 5 Attachment 115412 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10483844 New failing tests: fast/dom/MediaStream/argument-types.html

Created attachment 115493 [details] Patch 6 (In reply to comment #34) > (From update of attachment 115412 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=115412&action=review > > I think we should land this patch so we can move forward. This patch is generally very good. I have one memory-safety concern abou the lifetime of the UserMediaClient, which I've noted below, but we can address that issue in a follow-up patch. Thanks again for iterating on this patch. > > > Source/WebCore/mediastream/UserMediaRequest.cpp:105 > > + if (m_client) { > > + m_client->cancelUserMediaRequest(this); > > + m_client = 0; > > + } > > What is the lifetime of the UserMediaClient? In particular, can a document out-live the client? Yes, there is a potential lifetime problem if the document can out-live the page. Small fixes in new patch: * Removed early return on null-client in Navigator (caused test to fail; not needed since the client is checked before used). * Added functionality to be able to delete the UserMediaClient when the page is destroyed.

Comment on attachment 115493 [details] Patch 6 Yes, the document can live for an arbitrarily long period of time. Thanks.

Comment on attachment 115493 [details] Patch 6 Clearing flags on attachment: 115493 Committed r100555: <http://trac.webkit.org/changeset/100555>

All reviewed patches have been landed. Closing bug.