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

[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 114706: Patch 4
https://bugs.webkit.org/attachment.cgi?id=114706&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.