[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 #12 from Steve Block <steveblock at google.com>  2011-04-14 10:40:43 PST ---
(From update of attachment 89599)
View in context: https://bugs.webkit.org/attachment.cgi?id=89599&action=review

> Source/WebCore/page/MediaStreamController.cpp:54
> +            it = m_requests.begin();

You're right that remove() does invalidate iterators. A neater solution might be to maintain the reverse mapping from MediaStreamFrameController* to Set<globalId>. Then you can look up the set of map entries to delete.

> Source/WebCore/page/MediaStreamController.h:49
> +    class Request {

Probably best to move the impl of this class to the cpp. Or better, just forward declare the type here.

> Source/WebCore/page/MediaStreamFrameController.cpp:55
> +    // This script execution context is the document referenced by the frame. It will

I think you could simplify this comment. You just need to say that the SEC is guaranteed to have the lifetime of the Frame and that it's used only to make the callback async.

> Source/WebCore/page/MediaStreamFrameController.cpp:66
> +    ASSERT(contains(requestId));

No need to assert when we'd crash immediately afterwards anyway. Use asserts to highlight more subtle requirements.

> Source/WebCore/page/MediaStreamFrameController.cpp:73
> +    for (iterator it = begin(); it != end(); it = begin()) {

Strange construct. 'while (!empty())' is more usual.

> Source/WebCore/page/MediaStreamFrameController.cpp:113
> +// Called also on frame detachment, in which case the page controller will remain alive.

You mean when the Frame is detached from it's Page? Make this more clear.

> Source/WebCore/page/MediaStreamFrameController.cpp:123
> +void MediaStreamFrameController::disconnectFrame()

Make clear that we're doomed to at this point.

> Source/WebCore/page/MediaStreamFrameController.h:70
> +    class RequestMap : public IdGenerator, public HashMap<int, RefPtr<Request> > {

Why does RequestMap implement IdGenerator? I'd expect getNextId() to be an impl detail of the map. May be best to leave this class out for now, as I can't yet see how it's used.

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