[webkit-reviews] review granted: [Bug 81657] MediaStream API (JSEP): Introducing PeerConnection00 and IceCallback : [Attachment 132814] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Tommy Widenflycht
<tommyw at google.com>'s request for review:
Bug 81657: MediaStream API (JSEP): Introducing PeerConnection00 and IceCallback
https://bugs.webkit.org/show_bug.cgi?id=81657

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

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


> Source/WebCore/Modules/mediastream/PeerConnection00.cpp:294
> +    // FIXME(tommyw): When the spec says what the mediaStreamHints should
look like send it down.

FIXME(tommyw) => FIXME

> Source/WebCore/Modules/mediastream/PeerConnection00.cpp:404
> +bool PeerConnection00::canSuspend() const
> +{
> +    return false;
> +}

Isn't this the default?  I don't think you need to override this function.

> Source/WebCore/Modules/mediastream/PeerConnection00.cpp:450
> +    default:
> +	   ASSERT_NOT_REACHED();
> +	   break;

Normally we leave off the default case so that the compiler will complain if
you forget a case statement.

> Source/WebCore/Modules/mediastream/PeerConnection00.h:93
> +    PassRefPtr<SessionDescription> createOffer();
> +    PassRefPtr<SessionDescription> createOffer(const String& mediaHints);
> +    PassRefPtr<SessionDescription> createAnswer(const String& offer);
> +    PassRefPtr<SessionDescription> createAnswer(const String& offer, const
String& mediaHints);

You can use default parameter values if you'd rather.

> Source/WebCore/Modules/mediastream/PeerConnection00.h:134
> +    virtual bool canSuspend() const;
> +    virtual void stop();

Consider using OVERRIDE when overriding virtual methods.  It's not required,
but we've been using it more and more.

> Source/WebCore/Modules/mediastream/PeerConnection00.idl:40
> +	   // FIXME(tommyw): Make mediaHints an object

FIXME(tommyw) => FIXME

For whatever reason, WebKit doesn't put user names in FIXME.