Bug 92380

Created attachment 154647 [details] Patch

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

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 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:".."}, ... ]}

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

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

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.

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.

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

Created attachment 154887 [details] Patch

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?

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

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

Created attachment 154924 [details] Patch

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

Created attachment 154939 [details] Patch

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

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?

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

Created attachment 155259 [details] Patch

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

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

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

Created attachment 155296 [details] Patch

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

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 on attachment 155296 [details] Patch Attachment 155296 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13381812

Created attachment 155465 [details] Patch

Comment on attachment 155465 [details] Patch LGTM

Comment on attachment 155465 [details] Patch Clearing flags on attachment: 155465 Committed r124193: <http://trac.webkit.org/changeset/124193>

All reviewed patches have been landed. Closing bug.