[webkit-reviews] review denied: [Bug 80589] MediaStream API: Change the PeerConnection to use JSEP instead of ROAP : [Attachment 130808] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Tommy Widenflycht
<tommyw at google.com>'s request for review:
Bug 80589: MediaStream API: Change the PeerConnection to use JSEP instead of
ROAP
https://bugs.webkit.org/show_bug.cgi?id=80589

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=130808&action=review


This patch is much to big to review.  I noted a few minor issues.  Mostly we
need to divide this patch into many smaller pieces.  For example, I'd start
with one patch that just renames the existing APIs.  Then I'd introduce the new
interfaces in a series of patches.

> Source/WebCore/Modules/mediastream/IceCallback.h:43
> +class IceCallback : public RefCounted<IceCallback> {

I would have expected this to inherit from one of our callback base classes,
but maybe that's not necessary anymore.

> Source/WebCore/Modules/mediastream/IceCandidate.h:61
> +    RefPtr<IceCandidateBase> m_base;

Why isn't this a base class?

> Source/WebCore/Modules/mediastream/IceCandidate.idl:41
> +	   // the m= line this candidate is associated with
> +	   readonly attribute DOMString label;
> +
> +	   // creates a SDP-ized form of this candidate
> +	   DOMString toSdp();

Generally we write comments like these in IDL files.  That's what the specs are
for.  :)

> Source/WebCore/Modules/mediastream/MediaHints.h:60
> +    RefPtr<MediaHintsBase> m_base;

I don't really understand this pattern.  Why are these "base" objects being
held as members?

> Source/WebCore/Modules/mediastream/PeerConnection.cpp:375
> +bool PeerConnection::canSuspend() const
> +{
> +    return true;
> +}

Suspending is really complex.  Are you sure you're implementing this correctly?
 I'd recommend just returning false for now and worrying about suspension
later.