Bug 56922

Created attachment 86621 [details] Patch

Created attachment 86677 [details] Patch

(In reply to comment #2) > Created an attachment (id=86677) [details] > Patch Rebasing and changing raw pointers to the corresponding RefPtr and PassRefPtr objects.

Created attachment 88672 [details] Patch

Created attachment 88823 [details] Patch Rebasing and fixing license copyrights.

Comment on attachment 88823 [details] Patch 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? > Source/WebCore/page/MediaStreamContext.cpp:56 > + ScriptExecutionContext* m_scriptContext; Use full name - m_scriptExecutionContext > Source/WebCore/page/MediaStreamContext.cpp:105 > +void MediaStreamContext::pageDetached() As discussed, this method probably isn't needed right now. > Source/WebCore/page/MediaStreamContext.cpp:116 > +// Called on frame destruction despite the name. Remove > Source/WebCore/page/MediaStreamContext.cpp:131 > + if (pageController() == controller) When can the controller not be our controller? > Source/WebCore/page/MediaStreamContext.h:44 > +class MediaStreamContext { Try to avoid generic names like Context - maybe MediaStreamFrameController ? > Source/WebCore/page/MediaStreamContext.h:61 > + class IdGenerator { Why is this a separate class? > Source/WebCore/page/MediaStreamContext.h:77 > + void enterDetachedState(); Add a description of what it means to be detached. > Source/WebCore/page/MediaStreamContext.h:81 > + bool m_detachedState; m_isInDetachedState > 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. > Source/WebCore/page/MediaStreamController.h:45 > + void contextDetached(MediaStreamContext*); Make more descriptive - removeContentForFrameController > Source/WebCore/page/MediaStreamController.h:46 > + void transferContext(MediaStreamContext*, MediaStreamController*); transferContentForFrameController > 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. > Source/WebCore/page/MediaStreamController.h:67 > + typedef HashMap<int, RequestContext> RequestContextMap; keep typedef with with first use

Created attachment 89253 [details] Patch

Comment on attachment 88823 [details] Patch 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.

Created attachment 89599 [details] Patch

(In reply to comment #9) > Created an attachment (id=89599) [details] > Patch Fixing a problem in MediaStreamController::unregisterFrameController.

Comment on attachment 89599 [details] Patch 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.

Comment on attachment 89599 [details] Patch 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.

Created attachment 89624 [details] Patch

Comment on attachment 89599 [details] Patch 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.

Comment on attachment 89624 [details] Patch 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.

Created attachment 92069 [details] Patch

Comment on attachment 89624 [details] Patch 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.

Created attachment 92210 [details] Patch Just rebasing to avoid any problems in the commit queue.

Comment on attachment 92210 [details] Patch Clearing flags on attachment: 92210 Committed r85745: <http://trac.webkit.org/changeset/85745>

All reviewed patches have been landed. Closing bug.