[webkit-reviews] review requested: [Bug 68460] Add WebCore platform interfaces needed by updated PeerConnection design : [Attachment 110163] Patch 3

[bugzilla-daemon at webkit.org]

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

Attachment 110163: Patch 3
https://bugs.webkit.org/attachment.cgi?id=110163&action=review

------- Additional Comments from Adam Bergkvist <adam.bergkvist at ericsson.com>
(In reply to comment #28)
> (From update of attachment 109780 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=109780&action=review
> 
> Thanks for updating the patch.  I feel like we're making progress here.  My
two main concerns with this patch are MediaStreamCenter, which I don't really
understand, and the fact that your objects have a bunch of public member
variables.

Thank you for a quick review.

> One way to proceed is to not add MediaStreamCenter yet and add it when needed
by later patches.  That will help me understand it's role.

Let's do that. I've updated the name of this bug and I'll create a new one for
the MediaStream platform stuff. I'll address the review comments about
MediaStreamCenter in the new bug.

> As for the structs with public data members, can we start out with these
private and make them public as needed in future patches?  That will help us
see whether they actually need to be public or whether we've be better served
by having them private and exposing methods that manipulate them.

You're right, there are a few mebers that can be read-only. I've changed the
structs to classes (with private data members) and added the necessary getters
and setters.

> > Source/WebCore/platform/mediastream/MediaStreamComponent.h:54
> > +	 { }
> 
> Nit: These should be on separate lines.

Fixed.
 
> > Source/WebCore/platform/mediastream/MediaStreamDescriptor.h:63
> > +	 { }

Fixed.

> Nit: These should be on separate lines.
> 
> > Source/WebCore/platform/mediastream/MediaStreamSource.h:65
> > +	 { }
> 
> Nit: These should be on separate lines.

Fixed.

> > Source/WebCore/platform/mediastream/PeerHandler.cpp:41
> > +// FIXME: remove when real implementations are available
> > +// Empty implementations for ports that build with MEDIA_STREAM enabled by
default.
> > +#if PLATFORM(CHROMIUM) || PLATFORM(GTK)
> 
> I would just remove this ifdef.  We'd not going to need it.

That works. When a port has its own implementations it can opt out from using
these empty ones.