[Webkit-unassigned] [Bug 70897] Use a simple page client/controller for user consent in getUserMedia()

Tue Nov 8 17:01:16 PST 2011

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

Adam Bergkvist <adam.bergkvist at ericsson.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #113144|0                           |1
        is obsolete|                            |
 Attachment #114178|                            |review?
               Flag|                            |

--- Comment #10 from Adam Bergkvist <adam.bergkvist at ericsson.com>  2011-11-08 17:01:15 PST ---
Created an attachment (id=114178)
 --> (https://bugs.webkit.org/attachment.cgi?id=114178&action=review)
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.

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