Bug 92380 - Introduce a minimal RTCPeerConnection together with Dictionary changes
Summary: Introduce a minimal RTCPeerConnection together with Dictionary changes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Tommy Widenflycht
URL:
Keywords:
Depends on:
Blocks: 80589
  Show dependency treegraph
 
Reported: 2012-07-26 07:04 PDT by Tommy Widenflycht
Modified: 2012-07-31 05:00 PDT (History)
10 users (show)

See Also:


Attachments
Patch (3.30 KB, patch)
2012-07-26 07:09 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (24.00 KB, patch)
2012-07-27 02:49 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (30.55 KB, patch)
2012-07-27 06:17 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (30.38 KB, patch)
2012-07-27 07:16 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (42.03 KB, patch)
2012-07-30 04:48 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (47.48 KB, patch)
2012-07-30 08:22 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (48.77 KB, patch)
2012-07-31 01:31 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tommy Widenflycht 2012-07-26 07:04:18 PDT
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:".."}, ... ]}
Comment 1 Tommy Widenflycht 2012-07-26 07:09:37 PDT
Created attachment 154647 [details]
Patch
Comment 2 Tommy Widenflycht 2012-07-26 07:13:02 PDT
I would be grateful if someone could describe how to add tests for the class Dictionary.
Comment 3 Kentaro Hara 2012-07-26 07:30:57 PDT
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.
Comment 4 Tommy Widenflycht 2012-07-26 07:39:14 PDT
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:".."}, ... ]}
Comment 5 Tommy Widenflycht 2012-07-26 07:42:42 PDT
(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.
Comment 6 Kentaro Hara 2012-07-26 07:43:44 PDT
(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.
Comment 7 Tommy Widenflycht 2012-07-26 07:52:40 PDT
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.
Comment 8 Kentaro Hara 2012-07-26 07:58:55 PDT
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.
Comment 9 Kentaro Hara 2012-07-26 08:00:40 PDT
Also please add the spec link to the ChangeLog. This one? http://dev.w3.org/2011/webrtc/editor/webrtc.html#rtcpeerconnection
Comment 10 Tommy Widenflycht 2012-07-27 02:49:42 PDT
Created attachment 154887 [details]
Patch
Comment 11 Adam Barth 2012-07-27 03:02:43 PDT
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?
Comment 12 Adam Barth 2012-07-27 03:03:26 PDT
The RTCPeerConnection part of this patch looks fine.  My main concern is making the array support in dictionary efficient.
Comment 13 Kentaro Hara 2012-07-27 04:02:54 PDT
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?
Comment 14 Tommy Widenflycht 2012-07-27 06:14:37 PDT
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.
Comment 15 Tommy Widenflycht 2012-07-27 06:17:33 PDT
Created attachment 154924 [details]
Patch
Comment 16 Kentaro Hara 2012-07-27 06:41:54 PDT
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.
Comment 17 Tommy Widenflycht 2012-07-27 07:15:18 PDT
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.
Comment 18 Tommy Widenflycht 2012-07-27 07:16:37 PDT
Created attachment 154939 [details]
Patch
Comment 19 Tommy Widenflycht 2012-07-27 07:17:46 PDT
I am now working on mimicking the V8 changes to JSC, but that will likely not be done today.
Comment 20 Kentaro Hara 2012-07-27 07:25:39 PDT
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?
Comment 21 Tommy Widenflycht 2012-07-27 07:49:16 PDT
(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!
Comment 22 Kentaro Hara 2012-07-27 08:43:45 PDT
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.)
Comment 23 Tommy Widenflycht 2012-07-30 04:48:15 PDT
Created attachment 155259 [details]
Patch
Comment 24 Tommy Widenflycht 2012-07-30 04:49:45 PDT
I have now added the JSC variant as well, no changes in previously reviewed code.
Comment 25 Build Bot 2012-07-30 05:29:56 PDT
Comment on attachment 155259 [details]
Patch

Attachment 155259 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13381755
Comment 26 Kentaro Hara 2012-07-30 05:44:47 PDT
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.
Comment 27 Tommy Widenflycht 2012-07-30 05:51:07 PDT
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.
Comment 28 Kentaro Hara 2012-07-30 06:08:53 PDT
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().
Comment 29 Tommy Widenflycht 2012-07-30 07:06:27 PDT
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.
Comment 30 Tommy Widenflycht 2012-07-30 08:22:27 PDT
Created attachment 155296 [details]
Patch
Comment 31 Tommy Widenflycht 2012-07-30 08:23:58 PDT
Fixed several build files and refactored js/ArrayValue to behave more like js/Dictionary.
Comment 32 Kentaro Hara 2012-07-30 08:42:49 PDT
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.
Comment 33 Build Bot 2012-07-30 08:54:01 PDT
Comment on attachment 155296 [details]
Patch

Attachment 155296 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13381812
Comment 34 Tommy Widenflycht 2012-07-31 01:31:29 PDT
Created attachment 155465 [details]
Patch
Comment 35 Kentaro Hara 2012-07-31 02:00:11 PDT
Comment on attachment 155465 [details]
Patch

LGTM
Comment 36 WebKit Review Bot 2012-07-31 04:59:54 PDT
Comment on attachment 155465 [details]
Patch

Clearing flags on attachment: 155465

Committed r124193: <http://trac.webkit.org/changeset/124193>
Comment 37 WebKit Review Bot 2012-07-31 05:00:02 PDT
All reviewed patches have been landed.  Closing bug.