[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 #14 from Leandro Graciá Gil <leandrogracia at chromium.org>  2011-04-14 12:32:00 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();
>> 
>> I'm not sure if this is the best way to do this. Are HashMap iterators valid after another element from the map has been removed? If that's the case, I can just make a copy of the iterator, advance the original iterator to the next element and operate (possibly delete) the copy. That should avoid coming back to the beginning of the map everytime.
> 
> 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.

This loop is going to be repeated for Streams and, unlikely but possible, for something else in the future. I think that introducing these reverse maps adds an unnecessary extra complexity and more cleanups that can make the code even dirtier. I propose to use auxiliary vectors to find the keys to be removed and then remove them in a 2nd pass. This both avoids adding extra complexity and solves the problem of restarting the iteration for pages with many frames.

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

Fixed.

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

Done.

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

Fixed.

>> Source/WebCore/page/MediaStreamFrameController.cpp:73
>> +    for (iterator it = begin(); it != end(); it = begin()) {
> 
> Strange construct. 'while (!empty())' is more usual.

This follows the model of ScriptExecutionContext's destructor. In any case I have adopted your proposal.

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

Done.

>> Source/WebCore/page/MediaStreamFrameController.cpp:123
>> +void MediaStreamFrameController::disconnectFrame()
> 
> Make clear that we're doomed to at this point.

Done. Also refactored the code a bit.

> Source/WebCore/page/MediaStreamFrameController.cpp:140
> +    enterDetachedState();

Refactored.

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

Fixed. It will be introduced with its use in the next patch.

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