[webkit-reviews] review requested: [Bug 70895] Add WebCore platform interface needed by updated MediaStream API design : [Attachment 116089] Updated patch

[bugzilla-daemon at webkit.org]

Adam Bergkvist <adam.bergkvist at ericsson.com> has asked	for review:
Bug 70895: Add WebCore platform interface needed by updated MediaStream API
design
https://bugs.webkit.org/show_bug.cgi?id=70895

Attachment 116089: Updated patch
https://bugs.webkit.org/attachment.cgi?id=116089&action=review

------- Additional Comments from Adam Bergkvist <adam.bergkvist at ericsson.com>
(In reply to comment #4)
> Does MediaStreamCenter keep any state?  If not, perhaps it should be a
collection of free functions.

Yes, e.g., each media capture device is only represented by a single
MediaStreamSource instance so the MediaStreamCenter must keep track of the
MediaStreamSource instances it has created.

(In reply to comment #5)
> (From update of attachment 115608 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=115608&action=review
> 
> > Source/WebCore/platform/mediastream/MediaStreamCenter.cpp:38
> > +#include "MediaStream.h"
> 
> Another issue is that MediaStream.h is outside the Platform directory and
Platform isn't allowed to include headers from the rest of WebCore.

Fixed (introduced MediaStreamDescriptorOwner on the platform level). Fixed
similar issue with UserMediaRequest.

> > Source/WebCore/platform/mediastream/MediaStreamCenter.cpp:48
> > +MediaStreamCenter& MediaStreamCenter::instance()
> > +{
> > +	 ASSERT(isMainThread());
> > +	 DEFINE_STATIC_LOCAL(MediaStreamCenter, center, ());
> > +	 return center;
> > +}
> 
> The reason I ask these questions is that "static" is almost certainly the
wrong scope for any state in WebKit.

One reason it's static is to allow MediaStreamSource instances (which
correspond to specific media capture devices) to out-live everything else. A
MediaStreamSource can be used by several unrelated pages and muting a source
takes effect in all pages.