[webkit-reviews] review denied: [Bug 92380] Introduce a minimal RTCPeerConnection together with Dictionary changes : [Attachment 154887] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Tommy Widenflycht
<tommyw at google.com>'s request for review:
Bug 92380: Introduce a minimal RTCPeerConnection together with Dictionary
changes
https://bugs.webkit.org/show_bug.cgi?id=92380

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

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=154887&action=review


Marking r- due to inefficient Dictionary APIs

> Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:33
> +    // FIXME: The second argument to the constructor should be optional but
the idl code generator doesn't support that yet.

Can't you use [Optional=DefaultIsUndefined]?
(https://trac.webkit.org/wiki/WebKitIDL#Optional)

>> Source/WebCore/bindings/v8/Dictionary.cpp:441
>> +bool Dictionary::getArrayItem(const String& key, size_t index, Dictionary&
value) const
> 
> This seems inefficient since we need to look up the key each time.  Is there
a way to create some sort of temporary object that caches the
v8::Local<v8::Array> handle?

Yes. A general solution would be to introduce v8/Array.{h,cpp} and
js/Array.{h,cpp}. The Array class will provide Array::item(size_t index) and
Array::length(). Then you can add Dictionary::get(const String& key, Array&
array).

> Source/WebCore/bindings/v8/Dictionary.cpp:455
> +    if (!indexedValue->IsObject())

This check should be:

    if (!indexedValue->IsEmpty() || !indexedValue->IsObject())

> Source/WebCore/bindings/v8/Dictionary.h:91
> +    bool getArrayLength(const String& key, size_t& length) const;
> +    bool getArrayItem(const String& key, size_t index, Dictionary& value)
const;

Nit: WebKit omits obvious variable names in method declarations. 'key' and
'value' are not needed here.

> LayoutTests/fast/mediastream/RTCPeerConnection.html:4
> +<link rel="stylesheet" href="../js/resources/js-test-style.css">

Nit: Why is it needed?

> LayoutTests/fast/mediastream/RTCPeerConnection.html:9
> +<p id="description"></p>
> +<div id="console"></div>

Nit: These two lines are not needed. They will be generated by js-test-pre.js.

> LayoutTests/fast/mediastream/RTCPeerConnection.html:13
> +shouldNotThrow("new webkitRTCPeerConnection(null, null);");

In addition to null, please add test cases for undefined, '' (an empty string),
and a missing argument.

> LayoutTests/fast/mediastream/RTCPeerConnection.html:25
> +
> +window.jsTestIsAsync = false;

Is it needed?