Use a simple page client for user consent in getUserMedia()

[Adam Bergkvist]

The client should only be used for user consent when there are platform interfaces to do the low-level platform specific calls.

[Tommy Widenflycht]

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.

[Adam Bergkvist]

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

[Adam Bergkvist]

Created attachment 113144 [details]
Proposed patch

[WebKit Review Bot]

Comment on attachment 113144 [details]
Proposed patch

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

[Tommy Widenflycht]

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",

" -> '

[Tommy Widenflycht]

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.

[Tommy Widenflycht]

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

[Tommy Widenflycht]

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.

[Adam Barth]

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?

[Adam Bergkvist]

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.

[Adam Barth]

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?

[Adam Barth]

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

[Tommy Widenflycht]

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.

[Tommy Widenflycht]

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

[Tommy Widenflycht]

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!

[Adam Barth]

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

[Tommy Widenflycht]

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

[Adam Bergkvist]

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

[Tommy Widenflycht]

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

[Adam Bergkvist]

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.

[Tommy Widenflycht]

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

[Tommy Widenflycht]

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.

[WebKit Review Bot]

Comment on attachment 114476 [details]
Patch 3

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

[Adam Bergkvist]

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

[Tommy Widenflycht]

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

[Tommy Widenflycht]

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.

[Adam Barth]

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

[Adam Bergkvist]

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.

[Adam Barth]

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.

[Adam Barth]

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

[Adam Bergkvist]

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

[Adam Barth]

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

[Adam Bergkvist]

Created attachment 115412 [details]
Patch 5

Removed the controller.

[Adam Barth]

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?

[WebKit Review Bot]

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

[Adam Bergkvist]

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.

[Adam Barth]

Comment on attachment 115493 [details]
Patch 6

Yes, the document can live for an arbitrarily long period of time.

Thanks.

[WebKit Review Bot]

Comment on attachment 115493 [details]
Patch 6

Clearing flags on attachment: 115493

Committed r100555: <http://trac.webkit.org/changeset/100555>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.