[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 #17 from Leandro Graciá Gil <leandrogracia at chromium.org>  2011-05-03 06:55:09 PST ---
(From update of attachment 89624)
View in context: https://bugs.webkit.org/attachment.cgi?id=89624&action=review

>> Source/WebCore/page/MediaStreamClient.h:34
>> +    // Notify the client about shutting down.
> 
> Nit: this comment seems extraneous.

Replaced with 'Notify the embedder client about the page controller being destroyed'. Is this better?

>> Source/WebCore/page/MediaStreamController.cpp:38
>> +    Request()
> 
> Is this ctor just required for HashMap?

Yes, that's right. Do you think it can be removed?

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

For some reason it seems that m_requests.end() is returning an iterator instead of a const_iterator, producing a series of compile errors. The only way I managed to find to fix this is changing the comparison to end with it != static_cast<const RequestMap&>(m_requests).end(), which is very ugly. Since there's no clear benefit to keep trying to make it a const_iterator I'm leaving it as it is.

>> Source/WebCore/page/MediaStreamController.cpp:81
>> +{
> 
> Can we make any useful ASSERTs here? e.g. ASSERT(localId > 0) or ASSERT(frameController).

Fixed.

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

Fixed.

>> Source/WebCore/page/MediaStreamController.h:57
>> +    int m_nextGlobalRequestId;
> 
> Nit, but this should probably be unsigned.

This follows the example of Speech Input and Geolocation, where all the ids are ints. Changing this would require in practice to change all id types from int to size_t. It can be done, but I'm not sure if we should make this different to all others because of a nit. Maybe we should leave this to a later refactoring of the page clients?

>> 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)?

It will be used to schedule a call to the callback. This follows the example of the StringCallback class.
About asserting, no, we shouldn't as there are situations (in the detached frame state for example) where the scriptExecutionContext will be NULL. In this case, it just won't dispatch the corresponding error callback if any.

>> Source/WebCore/page/MediaStreamFrameController.cpp:102
>> +    if (m_isInDetachedState)
> 
> Since we are skipping abortAll, should we ASSERT(m_requests.isEmpty()) here?

Done.

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

The comment is right about transferring frames. I've added 'between pages' to the end to make it clearer.

>> Source/WebCore/page/MediaStreamFrameController.h:43
>> +class SecurityOrigin;
> 
> Some extra forward declaring here, maybe just trim to what is needed for now.

Fixed.

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