[Webkit-unassigned] [Bug 56922] Media Stream API: add the skeleton of the frame and page controllers and the embedder client.

[bugzilla-daemon at webkit.org]

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





--- Comment #15 from Tony Gentilcore <tonyg at chromium.org>  2011-04-21 03:48:39 PST ---
(From update of attachment 89624)
View in context: https://bugs.webkit.org/attachment.cgi?id=89624&action=review

Just a few nits.

When you get the chance, I'd like to ask you and Steve about the ownership model here. It seems slightly sad that we need two controllers, one for the frame and one for the page, but maybe that is unavoidable because of the async nature and possible desire to keep a stream running across frames.

> Source/WebCore/page/MediaStreamClient.h:34
> +    // Notify the client about shutting down.

Nit: this comment seems extraneous.

> Source/WebCore/page/MediaStreamController.cpp:38
> +    Request()

Is this ctor just required for HashMap?

> Source/WebCore/page/MediaStreamController.cpp:72
> +    for (RequestMap::iterator it = m_requests.begin(); it != m_requests.end(); ++it)

So this can be a const_iterator now, right?

> Source/WebCore/page/MediaStreamController.cpp:81
> +{

Can we make any useful ASSERTs here? e.g. ASSERT(localId > 0) or ASSERT(frameController).

> Source/WebCore/page/MediaStreamController.h:30
> +#include "NavigatorUserMediaError.h"

This include seems unnecessary here and also in a couple of other places. Maybe add it once it is needed?

> Source/WebCore/page/MediaStreamController.h:57
> +    int m_nextGlobalRequestId;

Nit, but this should probably be unsigned.

> Source/WebCore/page/MediaStreamFrameController.cpp:45
> +        : m_scriptExecutionContext(scriptExecutionContext) { }

I don't quite understand how this call will be used yet, but should we ASSERT(scriptExecutionContext)?

> Source/WebCore/page/MediaStreamFrameController.cpp:102
> +    if (m_isInDetachedState)

Since we are skipping abortAll, should we ASSERT(m_requests.isEmpty()) here?

> Source/WebCore/page/MediaStreamFrameController.cpp:130
> +    // FIXME: in the future we should keep running the media stream services while transfering frames.

s/in/In/
s/frames/pages/

> Source/WebCore/page/MediaStreamFrameController.h:43
> +class SecurityOrigin;

Some extra forward declaring here, maybe just trim to what is needed for now.

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