[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 #8 from Leandro Graciá Gil <leandrogracia at chromium.org>  2011-04-12 13:02:18 PST ---
(From update of attachment 88823)
View in context: https://bugs.webkit.org/attachment.cgi?id=88823&action=review

>> Source/WebCore/page/DOMWindow.h:503
>> +        OwnPtr<MediaStreamContext> m_mediaStreamContext;
> 
> Can he controller own a set of these, rather than polluting DOMWindow?

No, since we can have a frame without a page. These are the frames in the detached state. I'm moving this to the Frame class.

>> Source/WebCore/page/MediaStreamContext.cpp:56
>> +    ScriptExecutionContext* m_scriptContext;
> 
> Use full name - m_scriptExecutionContext

Fixed.

>> Source/WebCore/page/MediaStreamContext.cpp:105
>> +void MediaStreamContext::pageDetached()
> 
> As discussed, this method probably isn't needed right now.

It seems it is. This is called from Frame::pageDestroyed, which is called not only by the Page's destructor but also by FrameLoader::detachFromParent. In this last case the page is not really destroyed despite its name (there is a FIXME about this), so we need to tell the page controller to stop forwarding request replies to us.

>> Source/WebCore/page/MediaStreamContext.cpp:116
>> +// Called on frame destruction despite the name.
> 
> Remove

Changed to 'Called when the frame is being destroyed'.

>> Source/WebCore/page/MediaStreamContext.cpp:131
>> +    if (pageController() == controller)
> 
> When can the controller not be our controller?

This method is being deleted as it's not required for the moment.

>> Source/WebCore/page/MediaStreamContext.h:44
>> +class MediaStreamContext {
> 
> Try to avoid generic names like Context - maybe MediaStreamFrameController ?

Done. Changed to MediaStreamFrameController everywhere.

>> Source/WebCore/page/MediaStreamContext.h:61
>> +    class IdGenerator {
> 
> Why is this a separate class?

Because the next patch will introduce a new hash map that will inherit from it.

>> Source/WebCore/page/MediaStreamContext.h:77
>> +    void enterDetachedState();
> 
> Add a description of what it means to be detached.

Done.

>> Source/WebCore/page/MediaStreamContext.h:81
>> +    bool m_detachedState;
> 
> m_isInDetachedState

Fixed.

>> Source/WebCore/page/MediaStreamController.cpp:83
>> +void MediaStreamController::transferContext(MediaStreamContext* context, MediaStreamController* newController)
> 
> Given that this method isn't implemented properly yet, let's not add it yet to avoid bloat. transferToNewPage() can call contextDestroyed() directly.

Done. This method is now deleted.

>> Source/WebCore/page/MediaStreamController.h:45
>> +    void contextDetached(MediaStreamContext*);
> 
> Make more descriptive - removeContentForFrameController

Now using unregisterFrameController. Is that OK?

>> Source/WebCore/page/MediaStreamController.h:46
>> +    void transferContext(MediaStreamContext*, MediaStreamController*);
> 
> transferContentForFrameController

Method deleted as part of a previous review comment.

>> Source/WebCore/page/MediaStreamController.h:49
>> +    class RequestContext {
> 
> If this class is just for internal use, I don't think there's much need for it. Otherwsie, add an id() method to MediaStreamContext.

Renamed to Request and its id to localId to make clear its use. It is a map that translates from global per-page request id to local per-frame-controller request id, with its corresponding frame controller.

>> Source/WebCore/page/MediaStreamController.h:67
>> +    typedef HashMap<int, RequestContext> RequestContextMap;
> 
> keep typedef with with first use

Fixed.

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