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

[bugzilla-daemon at webkit.org]

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


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #113144|review?                     |review-
               Flag|                            |




--- Comment #9 from Adam Barth <abarth at webkit.org>  2011-11-01 09:25:46 PST ---
(From update of attachment 113144)
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?

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