[Webkit-unassigned] [Bug 68460] Add WebCore platform interfaces needed by updated PeerConnection design

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=68460


Adam Bergkvist <adam.bergkvist at ericsson.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #109780|0                           |1
        is obsolete|                            |
 Attachment #110163|                            |review?
               Flag|                            |




--- Comment #29 from Adam Bergkvist <adam.bergkvist at ericsson.com>  2011-10-07 09:56:13 PST ---
Created an attachment (id=110163)
 --> (https://bugs.webkit.org/attachment.cgi?id=110163&action=review)
Patch 3

(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.

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