[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 #114706|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #29 from Adam Barth <abarth at webkit.org>  2011-11-13 22:08:08 PST ---
(From update of attachment 114706)
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.

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