[webkit-reviews] review denied: [Bug 92380] Introduce a minimal RTCPeerConnection together with Dictionary changes : [Attachment 154924] 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 154924: Patch
https://bugs.webkit.org/attachment.cgi?id=154924&action=review

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


The approach looks great. I added a couple of minor comments.

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:87
> +    if (!ok || !iceServers.isArrayValue()) {

Is the !iceServers.isArrayValue() check needed?

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:104
> +	   if (!ok || !iceServer.isObject()) {

Is the !iceServer.isObject() needed?

> Source/WebCore/bindings/v8/ArrayValue.cpp:36
> +ArrayValue::ArrayValue()
> +{
> +}

Nit: You can write this in the header file.

> Source/WebCore/bindings/v8/ArrayValue.cpp:38
> +ArrayValue::ArrayValue(const v8::Local<v8::Value>& options)

Nit: You can write this in the header file.

> Source/WebCore/bindings/v8/ArrayValue.cpp:39
> +    : m_options(options)

m_options => m_array

> Source/WebCore/bindings/v8/ArrayValue.cpp:43
> +ArrayValue::~ArrayValue()

Nit: You can write this in the header file.

> Source/WebCore/bindings/v8/ArrayValue.cpp:47
> +ArrayValue& ArrayValue::operator=(const ArrayValue& optionsObject)

optionsObject => array

> Source/WebCore/bindings/v8/ArrayValue.cpp:70
> +    v8::Local<v8::Array> v8Array = v8::Local<v8::Array>::Cast(m_options);

How about storing v8Array instead of m_options to an ArrayValue object, so that
we can avoid casting m_options every time?

> Source/WebCore/bindings/v8/ArrayValue.cpp:84
> +    v8::Local<v8::Value> indexedValue =
v8Array->Get(v8::Uint32::New(index));

v8::Uint32::New(index) => v8UnsignedInteger(index)

> Source/WebCore/bindings/v8/ArrayValue.cpp:85
> +    if (WebCore::isUndefinedOrNull(indexedValue) ||
!indexedValue->IsObject())

WebCore::isUndefinedOrNull(indexedValue) => indexedValule.IsEmpty()  (Note:
This is not indexedValule->IsEmpty() but indexedValule.IsEmpty().)

> Source/WebCore/bindings/v8/ArrayValue.h:38
> +    ArrayValue(const v8::Local<v8::Value>& options);

Nit: 'options' is not needed.

> Source/WebCore/bindings/v8/ArrayValue.h:55
> +    v8::Local<v8::Value> m_options;

m_options => m_array

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

Let's add test cases for undefined and '' in the second argument.