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

[bugzilla-daemon at webkit.org]

Adam Bergkvist <adam.bergkvist at ericsson.com> has asked	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 114178: Updated patch
https://bugs.webkit.org/attachment.cgi?id=114178&action=review

------- Additional Comments from Adam Bergkvist <adam.bergkvist at ericsson.com>

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

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.