WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
56922
Media Stream API: add the skeleton of the frame and page controllers and the embedder client.
https://bugs.webkit.org/show_bug.cgi?id=56922
Summary
Media Stream API: add the skeleton of the frame and page controllers and the ...
Leandro Graciá Gil
Reported
2011-03-23 06:51:49 PDT
Add the basic outlines of the page controller for the Media Stream API and its client interface.
Attachments
Patch
(25.95 KB, patch)
2011-03-23 08:47 PDT
,
Leandro Graciá Gil
no flags
Details
Formatted Diff
Diff
Patch
(25.54 KB, patch)
2011-03-23 13:16 PDT
,
Leandro Graciá Gil
no flags
Details
Formatted Diff
Diff
Patch
(37.82 KB, patch)
2011-04-07 12:18 PDT
,
Leandro Graciá Gil
no flags
Details
Formatted Diff
Diff
Patch
(37.81 KB, patch)
2011-04-08 07:58 PDT
,
Leandro Graciá Gil
no flags
Details
Formatted Diff
Diff
Patch
(36.12 KB, patch)
2011-04-12 13:01 PDT
,
Leandro Graciá Gil
no flags
Details
Formatted Diff
Diff
Patch
(35.83 KB, patch)
2011-04-14 10:04 PDT
,
Leandro Graciá Gil
no flags
Details
Formatted Diff
Diff
Patch
(35.30 KB, patch)
2011-04-14 12:26 PDT
,
Leandro Graciá Gil
no flags
Details
Formatted Diff
Diff
Patch
(34.91 KB, patch)
2011-05-03 06:53 PDT
,
Leandro Graciá Gil
no flags
Details
Formatted Diff
Diff
Patch
(34.83 KB, patch)
2011-05-04 03:32 PDT
,
Leandro Graciá Gil
no flags
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Leandro Graciá Gil
Comment 1
2011-03-23 08:47:52 PDT
Created
attachment 86621
[details]
Patch
Leandro Graciá Gil
Comment 2
2011-03-23 13:16:15 PDT
Created
attachment 86677
[details]
Patch
Leandro Graciá Gil
Comment 3
2011-03-23 13:16:59 PDT
(In reply to
comment #2
)
> Created an attachment (id=86677) [details] > Patch
Rebasing and changing raw pointers to the corresponding RefPtr and PassRefPtr objects.
Leandro Graciá Gil
Comment 4
2011-04-07 12:18:44 PDT
Created
attachment 88672
[details]
Patch
Leandro Graciá Gil
Comment 5
2011-04-08 07:58:48 PDT
Created
attachment 88823
[details]
Patch Rebasing and fixing license copyrights.
Steve Block
Comment 6
2011-04-11 09:25:52 PDT
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
Leandro Graciá Gil
Comment 7
2011-04-12 13:01:01 PDT
Created
attachment 89253
[details]
Patch
Leandro Graciá Gil
Comment 8
2011-04-12 13:02:18 PDT
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.
Leandro Graciá Gil
Comment 9
2011-04-14 10:04:37 PDT
Created
attachment 89599
[details]
Patch
Leandro Graciá Gil
Comment 10
2011-04-14 10:05:52 PDT
(In reply to
comment #9
)
> Created an attachment (id=89599) [details] > Patch
Fixing a problem in MediaStreamController::unregisterFrameController.
Leandro Graciá Gil
Comment 11
2011-04-14 10:08:32 PDT
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.
Steve Block
Comment 12
2011-04-14 10:40:43 PDT
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.
Leandro Graciá Gil
Comment 13
2011-04-14 12:26:51 PDT
Created
attachment 89624
[details]
Patch
Leandro Graciá Gil
Comment 14
2011-04-14 12:32:00 PDT
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.
Tony Gentilcore
Comment 15
2011-04-21 03:48:39 PDT
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.
Leandro Graciá Gil
Comment 16
2011-05-03 06:53:28 PDT
Created
attachment 92069
[details]
Patch
Leandro Graciá Gil
Comment 17
2011-05-03 06:55:09 PDT
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.
Leandro Graciá Gil
Comment 18
2011-05-04 03:32:33 PDT
Created
attachment 92210
[details]
Patch Just rebasing to avoid any problems in the commit queue.
WebKit Commit Bot
Comment 19
2011-05-04 06:57:43 PDT
Comment on
attachment 92210
[details]
Patch Clearing flags on attachment: 92210 Committed
r85745
: <
http://trac.webkit.org/changeset/85745
>
WebKit Commit Bot
Comment 20
2011-05-04 06:57:50 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug