[webkit-reviews] review denied: [Bug 65101] MediaStream API: Implement PeerConnection and SignalingCallback : [Attachment 102753] 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 65101: MediaStream API: Implement PeerConnection and SignalingCallback
https://bugs.webkit.org/show_bug.cgi?id=65101

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

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


The Frame-finding code still isn't the same between JSC and V8, but I feel bad
bouncing your patch again.  I've also noted some minor nits that you shouldn't
feel obligated to fix.	If you'd like, you should feel free to update this
patch or deal with the remaining (minor) issues in a follow-up patch.

> Source/WebCore/bindings/v8/custom/V8PeerConnectionCustom.cpp:64
> +    Frame* frame = V8Proxy::retrieveFrameForEnteredContext();
> +    MediaStreamFrameController* frameController = frame ?
frame->mediaStreamFrameController() : 0;

It looks like you changed both of these.

retrieveFrameForEnteredContext <=>
asJSDOMWindow(exec->dynamicGlobalObject())->impl()->frame()

but you're using lexicalGlobalObject for the JSC bindings...  I'm sorry these
names are confusing.  The problem is that the V8 names come from the V8 API and
the JSC names come from the programming languages literature.  Neither group
wants to change their names to match the other.

> Source/WebCore/p2p/PeerConnection.cpp:104
> +    CString utf8 = text.utf8();
> +    if (utf8.length() > 504) {

Should we make 504 a named constant?  This check is strange, but I assume it's
from the spec.

> Source/WebCore/p2p/PeerConnection.cpp:182
> +    RefPtr<MediaStream> s = stream;

prpStream

> Source/WebCore/page/MediaStreamFrameController.cpp:407
> +    RefPtr<MediaStream> stream = streamRef;

streamRef => prpStream

> Source/WebCore/page/MediaStreamFrameController.cpp:416
> +    RefPtr<MediaStream> stream = streamRef;

ditto