Bug 92380 – Introduce a minimal RTCPeerConnection together with Dictionary changes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Tommy Widenflycht]

Since Dictionaries can't be fully copied around having an get(const String& name, Vector<Dictionary>& result isn't reasible so I have added two new methods: getArrayLength and getArrayItem.
This is just done for V8, and if this patch is approved by the powers I'll add the same to JS.

This is needed to be able to parse dictionaries created by these kinds of constructs:

{ servers: [ {uri:"...", credential:".."}, {uri:"...", credential:".."}, ... ]}

[Tommy Widenflycht]

Created attachment 154647 [details]
Patch

[Tommy Widenflycht]

I would be grateful if someone could describe how to add tests for the class Dictionary.

[Kentaro Hara]

Comment on attachment 154647 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154647&action=review

r- due to missing tests.

> Source/WebCore/ChangeLog:10
> +        isn't reasible so I have added two new methods: getArrayLength and getArrayItem.

I don't understand why you need these two methods. Please describe your working context here.

> Source/WebCore/ChangeLog:12
> +        No new tests (OOPS!).

Please add test cases not only to test your change but also to show how you are planning to use the methods.

> I would be grateful if someone could describe how to add tests for the class Dictionary.

There are no unit tests. You can add LayoutTests that use the methods.

[Tommy Widenflycht]

Comment on attachment 154647 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154647&action=review

>> Source/WebCore/ChangeLog:10
>> +        isn't reasible so I have added two new methods: getArrayLength and getArrayItem.
> 
> I don't understand why you need these two methods. Please describe your working context here.

This is needed to be able to parse dictionaries created by these kinds of constructs:
{ servers: [ {uri:"...", credential:".."}, {uri:"...", credential:".."}, ... ]}

[Tommy Widenflycht]

(In reply to comment #3)
> > Source/WebCore/ChangeLog:12
> > +        No new tests (OOPS!).
> 
> Please add test cases not only to test your change but also to show how you are planning to use the methods.
> 
> > I would be grateful if someone could describe how to add tests for the class Dictionary.
> 
> There are no unit tests. You can add LayoutTests that use the methods.

That requires adding the code that does the actual parsing, a mock, DumpRenderTree modifications and LayoutTests. I was hoping there were a way to just test this class separately.

[Kentaro Hara]

(In reply to comment #4)
> This is needed to be able to parse dictionaries created by these kinds of constructs:
> { servers: [ {uri:"...", credential:".."}, {uri:"...", credential:".."}, ... ]}

Specifically, where is it used in JavaScript? Would you write some code snippet you are assuming? I can give you more advice how to write the test.

[Tommy Widenflycht]

http://dev.w3.org/2011/webrtc/editor/webrtc.html#rtcconfiguration-type

When one wants to create a RTCPeerConnection object one has to specify which TURN/STUN servers to use like this

pc = new RTCPeerConnection({servers:[{uri:"...", credential:".."},{uri:"...",credential:".."},...]});

and a simple real world example would be

pc = new RTCPeerConnection({servers:[{uri:"stun.l.google.com"}]});

The peer connection WebCore native object needs to parse this and send down the list of servers to the network stack for the ICE process.

[Kentaro Hara]

OK, then a good example is event-constructors.html: http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/LayoutTests/fast/events/constructors/event-constructors.html&type=cs

You can add your test to the directory that contains tests about RTCPeerConnection or something like that.

[Kentaro Hara]

Also please add the spec link to the ChangeLog. This one? http://dev.w3.org/2011/webrtc/editor/webrtc.html#rtcpeerconnection

[Tommy Widenflycht]

Created attachment 154887 [details]
Patch

[Adam Barth]

Comment on attachment 154887 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154887&action=review

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:43
> +// FYI: RTCIceServer and RTCConfigration are placed here temporarily.
> +// Their final place is in Source/WebCore/platform/mediastream.

I probably would have phrased this as a FIXME comment.

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:53
> +    RTCIceServer(const String& uri, const String& password) : m_uri(uri), m_password(password) { }

This should be split onto a bunch of lines.

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:145
> +void RTCPeerConnection::stop()
> +{
> +}

Should we add a FIXME about what we should do here?

> 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?

[Adam Barth]

The RTCPeerConnection part of this patch looks fine.  My main concern is making the array support in dictionary efficient.

[Kentaro Hara]

Comment on attachment 154887 [details]
Patch

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?

[Tommy Widenflycht]

Comment on attachment 154887 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154887&action=review

>> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:43
>> +// Their final place is in Source/WebCore/platform/mediastream.
> 
> I probably would have phrased this as a FIXME comment.

Done.

>> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:53
>> +    RTCIceServer(const String& uri, const String& password) : m_uri(uri), m_password(password) { }
> 
> This should be split onto a bunch of lines.

Done.

>> 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)

That works just fine :)
Just optional doesn't work, but this is even better.

>>> 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).

The name Array is already taken so I choose ArrayValue.

>> Source/WebCore/bindings/v8/Dictionary.cpp:455
>> +    if (!indexedValue->IsObject())
> 
> This check should be:
> 
>     if (!indexedValue->IsEmpty() || !indexedValue->IsObject())

Fixed, but not quite like you wrote. indexedValue doesn't have a IsEmpty().

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

Dunno. Copy-n-paste...

>> LayoutTests/fast/mediastream/RTCPeerConnection.html:9
>> +<div id="console"></div>
> 
> Nit: These two lines are not needed. They will be generated by js-test-pre.js.

Fixed.

>> 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.

Done.

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

No, but I usually put it in for symmetry reasons.

[Tommy Widenflycht]

Created attachment 154924 [details]
Patch

[Kentaro Hara]

Comment on attachment 154924 [details]
Patch

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.

[Tommy Widenflycht]

Comment on attachment 154924 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154924&action=review

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

No, not anymore.

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

No.

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

Done.

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

Done.

>> 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?

Done.

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

Done.

>> 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().)

Doh. Thanks.

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

Fixed.

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

Fixed.

>> LayoutTests/fast/mediastream/RTCPeerConnection.html:10
>> +shouldNotThrow("new webkitRTCPeerConnection(null, null);");
> 
> Let's add test cases for undefined and '' in the second argument.

Done. The second argument will get proper parsing on its own later on and get some proper tests at that stage.

[Tommy Widenflycht]

Created attachment 154939 [details]
Patch

[Tommy Widenflycht]

I am now working on mimicking the V8 changes to JSC, but that will likely not be done today.

[Kentaro Hara]

Comment on attachment 154939 [details]
Patch

Looks good. r+ for all the files in this patch.

In my understanding, you need to land the js/Dictionary.{h,cpp} at the same time (and you are going to do it tomorrow), right?

[Tommy Widenflycht]

(In reply to comment #20)
> (From update of attachment 154939 [details])
> Looks good. r+ for all the files in this patch.
> 
> In my understanding, you need to land the js/Dictionary.{h,cpp} at the same time (and you are going to do it tomorrow), right?

That's right. Unless JSC has the same changes the GTK port will not compile. And unless I can't finish this patch today I will do it on Monday.

Thanks for your review!

[Kentaro Hara]

Comment on attachment 154939 [details]
Patch

Sounds like a good plan! (Marking r- due to missing JSC support. We just don't want to show the patch in the review list.)

[Tommy Widenflycht]

Created attachment 155259 [details]
Patch

[Tommy Widenflycht]

I have now added the JSC variant as well, no changes in previously reviewed code.

[Build Bot]

Comment on attachment 155259 [details]
Patch

Attachment 155259 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13381755

[Kentaro Hara]

Comment on attachment 155259 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155259&action=review

> Source/WebCore/bindings/js/ArrayValue.cpp:41
> +ArrayValue::ArrayValue(JSC::ExecState* exec, JSC::JSValue value)

value => array (for clarification and consistency with V8)

> Source/WebCore/bindings/js/ArrayValue.cpp:43
> +    , m_value(value)

m_value => m_array (for clarification and consistency with V8)

> Source/WebCore/bindings/js/ArrayValue.cpp:57
> +    return !m_exec;

This should be something like this:

  if (m_value.IsEmpty())
    return true;
  return WebCore::isUndefinedOrNull(m_value);

> Source/WebCore/bindings/js/ArrayValue.cpp:65
> +    JSArray* array = asArray(m_value);

Let's store JSArray* to the ArrayValue object so that we can avoid calling asArray every time.

Also you need to check m_value->isArray() before calling asArray(). Given that you store JSArray* to the ArrayValue object, you can do the check in JSDictionary::convertValue() before calling the ArrayValue constructor.

> Source/WebCore/bindings/js/ArrayValue.cpp:75
> +    JSArray* array = asArray(m_value);

Ditto.

> Source/WebCore/bindings/js/ArrayValue.h:29
> +#include <interpreter/CallFrame.h>

Remove this.

[Tommy Widenflycht]

Comment on attachment 155259 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155259&action=review

>> Source/WebCore/bindings/js/ArrayValue.cpp:57
>> +    return !m_exec;
> 
> This should be something like this:
> 
>   if (m_value.IsEmpty())
>     return true;
>   return WebCore::isUndefinedOrNull(m_value);

No need, I have an assert in the constructor.

>> Source/WebCore/bindings/js/ArrayValue.cpp:65
>> +    JSArray* array = asArray(m_value);
> 
> Let's store JSArray* to the ArrayValue object so that we can avoid calling asArray every time.
> 
> Also you need to check m_value->isArray() before calling asArray(). Given that you store JSArray* to the ArrayValue object, you can do the check in JSDictionary::convertValue() before calling the ArrayValue constructor.

Can't do that unless we store both the JSValue object and the JSArray pointer, just storing the pointer is dangerous.
And again I do have an assert in the constructor that stops anything else than Array objects.

[Kentaro Hara]

Comment on attachment 155259 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155259&action=review

> Source/WebCore/bindings/js/ArrayValue.cpp:45
> +    ASSERT(m_exec && !m_value.isUndefinedOrNull() && isJSArray(m_value));

Let's write the ASSERTs in separate lines.

  ASSERT(m_exec);
  ASSERT(!m_value.isUndefinedOrNull());
  ASSERT(isJSArray(m_value));

BTW, do you intend m_value.isUndefinedOrNull() or WebCore::isUndefinedOrNull(m_value)? (I think the latter would be better.)

>>> Source/WebCore/bindings/js/ArrayValue.cpp:57
>>> +    return !m_exec;
>> 
>> This should be something like this:
>> 
>>   if (m_value.IsEmpty())
>>     return true;
>>   return WebCore::isUndefinedOrNull(m_value);
> 
> No need, I have an assert in the constructor.

You might be right but I would like to make the code clearer.

For example, what happens in the following code?

  ArrayValue v;
  v.length();

The code will work well but the reasoning is not straightforward. Given that the code here would not be performance sensitive, let's write a straightforward code (i.e. the same code as ArrayValue::isUndefinedOrNull() in v8/ArrayValue.cpp)

>>> Source/WebCore/bindings/js/ArrayValue.cpp:65
>>> +    JSArray* array = asArray(m_value);
>> 
>> Let's store JSArray* to the ArrayValue object so that we can avoid calling asArray every time.
>> 
>> Also you need to check m_value->isArray() before calling asArray(). Given that you store JSArray* to the ArrayValue object, you can do the check in JSDictionary::convertValue() before calling the ArrayValue constructor.
> 
> Can't do that unless we store both the JSValue object and the JSArray pointer, just storing the pointer is dangerous.
> And again I do have an assert in the constructor that stops anything else than Array objects.

Makes sense. Thanks for the clarification. Then let's insert ASSERT(isJSArray(m_value)) into just before asArray().

>> Source/WebCore/bindings/js/ArrayValue.cpp:75
>> +    JSArray* array = asArray(m_value);
> 
> Ditto.

Let's insert ASSERT(isJSArray(m_value)) into just before asArray().

[Tommy Widenflycht]

Comment on attachment 155259 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155259&action=review

>> Source/WebCore/bindings/js/ArrayValue.cpp:45
>> +    ASSERT(m_exec && !m_value.isUndefinedOrNull() && isJSArray(m_value));
> 
> Let's write the ASSERTs in separate lines.
> 
>   ASSERT(m_exec);
>   ASSERT(!m_value.isUndefinedOrNull());
>   ASSERT(isJSArray(m_value));
> 
> BTW, do you intend m_value.isUndefinedOrNull() or WebCore::isUndefinedOrNull(m_value)? (I think the latter would be better.)

The code in bindings/js/ uses value.isUndefinedOrNull().

>> Source/WebCore/bindings/js/ArrayValue.h:29
>> +#include <interpreter/CallFrame.h>
> 
> Remove this.

Not going to. This brings in the definition of JSC::ExecState.

[Tommy Widenflycht]

Created attachment 155296 [details]
Patch

[Tommy Widenflycht]

Fixed several build files and refactored js/ArrayValue to behave more like js/Dictionary.

[Kentaro Hara]

Comment on attachment 155296 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155296&action=review

Looks good to me. Please confirm that build bots get green before landing.

> Source/WebCore/bindings/js/ArrayValue.cpp:44
> +    if (!value.isUndefinedOrNull() && isJSArray(value))

Nit: To keep the implementation consistent between JSC and V8, I might want to move this check to JSDictionary::convertValue(). You can write ASSERT() here.

> Source/WebCore/bindings/v8/ArrayValue.cpp:44
> +    if (m_array.IsEmpty())
> +        return true;
> +    return WebCore::isUndefinedOrNull(m_array);

Nit: This could be:

    return m_array.IsEmpty() || WebCore::isUndefinedOrNull(m_array);

Let's keep the implementation consistent between JSC and V8.

[Build Bot]

Comment on attachment 155296 [details]
Patch

Attachment 155296 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13381812

[Tommy Widenflycht]

Created attachment 155465 [details]
Patch

[Kentaro Hara]

Comment on attachment 155465 [details]
Patch

LGTM

[WebKit Review Bot]

Comment on attachment 155465 [details]
Patch

Clearing flags on attachment: 155465

Committed r124193: <http://trac.webkit.org/changeset/124193>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.