[webkit-reviews] review denied: [Bug 70897] Use a simple page client/controller for user consent in getUserMedia() : [Attachment 113144] Proposed patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Adam Bergkvist
<adam.bergkvist at ericsson.com>'s request for review:
Bug 70897: Use a simple page client/controller for user consent in
getUserMedia()
https://bugs.webkit.org/show_bug.cgi?id=70897

Attachment 113144: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=113144&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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_f
rame);
> +#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?