This requires supporting mapLike
Created attachment 298537 [details] WIP
Comment on attachment 298537 [details] WIP Attachment 298537 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2867171 Number of test failures exceeded the failure limit.
Created attachment 298548 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 298661 [details] Patch
Created attachment 298662 [details] Patch
Created attachment 298694 [details] Patch
Comment on attachment 298694 [details] Patch Attachment 298694 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2878259 New failing tests: webrtc/stats.html
Created attachment 298700 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 298694 [details] Patch Attachment 298694 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2878235 New failing tests: webrtc/stats.html
Created attachment 298701 [details] Archive of layout-test-results from ews116 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 298706 [details] Patch
Created attachment 298712 [details] Patch
Comment on attachment 298712 [details] Patch Attachment 298712 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2879934 New failing tests: fast/mediastream/MediaStream-video-element-track-stop.html
Created attachment 298745 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 298712 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298712&action=review > Source/WebCore/Modules/mediastream/RTCStatsReport.cpp:48 > + m_object->putDirect(vm, JSC::Identifier::fromString(&vm, "id"), toJS<IDLDOMString>(state, globalObject, m_id)); > + m_object->putDirect(vm, JSC::Identifier::fromString(&vm, "type"), toJS<IDLDOMString>(state, globalObject, String(type))); > + m_object->putDirect(vm, JSC::Identifier::fromString(&vm, "timestamp"), toJS<IDLUnsignedLongLong>(timestamp)); I'm not thrilled with having so much JS code in non-bindings code. Are there any other ways to accomplish this while still keeping a more substantial boundary?
(In reply to comment #15) > Comment on attachment 298712 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=298712&action=review > > > Source/WebCore/Modules/mediastream/RTCStatsReport.cpp:48 > > + m_object->putDirect(vm, JSC::Identifier::fromString(&vm, "id"), toJS<IDLDOMString>(state, globalObject, m_id)); > > + m_object->putDirect(vm, JSC::Identifier::fromString(&vm, "type"), toJS<IDLDOMString>(state, globalObject, String(type))); > > + m_object->putDirect(vm, JSC::Identifier::fromString(&vm, "timestamp"), toJS<IDLUnsignedLongLong>(timestamp)); > > I'm not thrilled with having so much JS code in non-bindings code. Are > there any other ways to accomplish this while still keeping a more > substantial boundary? For instance, why couldn't we just use a HashMap as the backing map?
Comment on attachment 298712 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298712&action=review > Source/WebCore/bindings/js/JSDOMBinding.cpp:1039 > + mapLike.putDirect(vm, static_cast<JSVMClientData*>(vm.clientData)->builtinNames().backingMapPrivateName(), JSMap::create(mapLike.globalObject()->globalExec(), vm, mapLike.globalObject()->mapStructure()), DontEnum); If we do go the JSMap as backing map route, do we need to do something to prevent the user from tampering with the prototype of the Map object (e.g. have an JSInternalMap, like we have JSInternalPromise)?
> I'm not thrilled with having so much JS code in non-bindings code. Are > there any other ways to accomplish this while still keeping a more > substantial boundary? We can move that code to bindings/js, something like a writable dictionary. > For instance, why couldn't we just use a HashMap as the backing map? We could but we would depart from https://heycam.github.io/webidl/#es-maplike. The goal of maplike is to be totally equivalent to es Map. For instance, iterators returned by maplike should be @MapIterator.
(In reply to comment #17) > Comment on attachment 298712 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=298712&action=review > > > Source/WebCore/bindings/js/JSDOMBinding.cpp:1039 > > + mapLike.putDirect(vm, static_cast<JSVMClientData*>(vm.clientData)->builtinNames().backingMapPrivateName(), JSMap::create(mapLike.globalObject()->globalExec(), vm, mapLike.globalObject()->mapStructure()), DontEnum); > > If we do go the JSMap as backing map route, do we need to do something to > prevent the user from tampering with the prototype of the Map object (e.g. > have an JSInternalMap, like we have JSInternalPromise)? From my quick reading of webidl, we are to use the public slot of the Map object. But it would probably be safer to use private slots on the Map prototype.
I'll fix the test and will improve the JS binding layering
Comment on attachment 298712 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298712&action=review > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:388 > + if (m_signalingState == SignalingState::Closed) > + promise.reject(INVALID_STATE_ERR); I think this is missing a return. Or am I mistaken? Do we have a test case covering this?
(In reply to comment #21) > Comment on attachment 298712 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=298712&action=review > > > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:388 > > + if (m_signalingState == SignalingState::Closed) > > + promise.reject(INVALID_STATE_ERR); > > I think this is missing a return. Or am I mistaken? Do we have a test case > covering this? That is right. LayoutTests/fast/mediastream/RTCPeerConnection-closed-state.html is covering this case. I am not sure why it has not caught it.
Created attachment 299067 [details] Patch
(In reply to comment #22) > (In reply to comment #21) > > Comment on attachment 298712 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=298712&action=review > > > > > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:388 > > > + if (m_signalingState == SignalingState::Closed) > > > + promise.reject(INVALID_STATE_ERR); > > > > I think this is missing a return. Or am I mistaken? Do we have a test case > > covering this? > > That is right. > LayoutTests/fast/mediastream/RTCPeerConnection-closed-state.html is covering > this case. > I am not sure why it has not caught it. Since the change to make promise active dom objects, a promise may be rejected/resolved twice without getting notice. The second time, it just exits early.
(In reply to comment #15) > Comment on attachment 298712 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=298712&action=review > > > Source/WebCore/Modules/mediastream/RTCStatsReport.cpp:48 > > + m_object->putDirect(vm, JSC::Identifier::fromString(&vm, "id"), toJS<IDLDOMString>(state, globalObject, m_id)); > > + m_object->putDirect(vm, JSC::Identifier::fromString(&vm, "type"), toJS<IDLDOMString>(state, globalObject, String(type))); > > + m_object->putDirect(vm, JSC::Identifier::fromString(&vm, "timestamp"), toJS<IDLUnsignedLongLong>(timestamp)); > > I'm not thrilled with having so much JS code in non-bindings code. Are > there any other ways to accomplish this while still keeping a more > substantial boundary? I added an UntypedDictionary. It might be useful in the future to have a kind of extensible IDLDictionary where additional key from the ones defined in the IDL could be put. In the case of WebRTC, this does not seem to be worth it since the dictionary information is not persistent as would be case for Dictionary attributes for instance.
This still feels like a step backwards. We spent a lot of effort to remove all the uses of Dictionary from WebCore, and this feels like it will have many of the same problems (e.g. incorrect dealing with exceptions / gc lifetimes). Can you explain again why this can't be backed by a HashMap or if needs to be ordered, some other map, where type conversions happen at the boundary?
(In reply to comment #26) > This still feels like a step backwards. We spent a lot of effort to remove > all the uses of Dictionary from WebCore, and this feels like it will have > many of the same problems (e.g. incorrect dealing with exceptions / gc > lifetimes). This patch is doing two things: 1. adding support for MapLike 2. adding support for stats API which uses a promise that resolves to a MapLike of extensible dictionaries In this discussion, we should decouple the two. For item 1, the maplike is stored as a slot to the JSXX object. There should be no gc issues. As of exceptions, they could happen when creating the backing map like and storing it as a slot. They could also happen when adding items. I can only imagine memory exception in both cases. In our specific case, the only thing we could do would be to reject the promise with the given exception. For item 2,we could use IDL dictionaries instead to build RTCStats items. But this would be more code/less efficient/less flexible in that specific case. We would create structures, fill them, convert them to JS, add them to map like and here we go. > Can you explain again why this can't be backed by a HashMap or if needs to > be ordered, some other map, where type conversions happen at the boundary? WebIDL is defining maplike exactly as implemented in this patch: an object exposing data stored into a backing JS map. To expose it properly as defined by WebIDL, we need to send back MapIterator objects to methods like values or entries. A MapIterator is bound to a JS Map. The alternative I considered at some point was to use JS built-ins to feed the RTCStatsReport. Instead of resolving a fed RTCStatsReport, we would resolve the promise with a vector containing the different RTCStats. The JS built-in would be responsible to fill the backing map slot with the provided vector. I did not go this way since it is less generic and creating a temporary array is less efficient than directly adding items to the map. It is true though that the exception handling would be probably nicer. wdyt?
(In reply to comment #27) > (In reply to comment #26) > > This still feels like a step backwards. We spent a lot of effort to remove > > all the uses of Dictionary from WebCore, and this feels like it will have > > many of the same problems (e.g. incorrect dealing with exceptions / gc > > lifetimes). > > This patch is doing two things: > 1. adding support for MapLike > 2. adding support for stats API which uses a promise that resolves to a > MapLike of extensible dictionaries > > In this discussion, we should decouple the two. > > For item 1, the maplike is stored as a slot to the JSXX object. > There should be no gc issues. > As of exceptions, they could happen when creating the backing map like and > storing it as a slot. They could also happen when adding items. I can only > imagine memory exception in both cases. This is only true of a readonly maplike object. Once C++ land has to read values out of it, conversion exceptions can happen if I am not mistaken. > In our specific case, the only thing we could do would be to reject the > promise with the given exception. > > For item 2,we could use IDL dictionaries instead to build RTCStats items. > But this would be more code/less efficient/less flexible in that specific > case. > We would create structures, fill them, convert them to JS, add them to map > like and here we go. This would be no different than all the rest of IDL infrastructure. A c++ backing, wrapped by a JavaScript object. > > > Can you explain again why this can't be backed by a HashMap or if needs to > > be ordered, some other map, where type conversions happen at the boundary? > > WebIDL is defining maplike exactly as implemented in this patch: an object > exposing data stored into a backing JS map. > To expose it properly as defined by WebIDL, we need to send back MapIterator > objects to methods like values or entries. A MapIterator is bound to a JS > Map. > Is any of that actually user observable, or is the spec just using the language of the ES spec to define the behavior. If for instance, modifications to the Map prototype effect the behavior of maplike objects, then it probably makes sense to implement it using the JS Map. But if not, I would like to stick to our normal patterns. > The alternative I considered at some point was to use JS built-ins to feed > the RTCStatsReport. > Instead of resolving a fed RTCStatsReport, we would resolve the promise with > a vector containing the different RTCStats. The JS built-in would be > responsible to fill the backing map slot with the provided vector. > I did not go this way since it is less generic and creating a temporary > array is less efficient than directly adding items to the map. It is true > though that the exception handling would be probably nicer. > > wdyt? I prefer keeping our pattern of c++ wrapped by js object for these.
> This is only true of a readonly maplike object. Once C++ land has to read > values out of it, conversion exceptions can happen if I am not mistaken. Right, I would expect WebCore binding code to actually enforce that key and value have the types defined by the maplike in the IDL. The storage in the backing map would happen in the binding code and WebCore regular code would get notified of the converted C++ key/value. > > In our specific case, the only thing we could do would be to reject the > > promise with the given exception. > > > > For item 2,we could use IDL dictionaries instead to build RTCStats items. > > But this would be more code/less efficient/less flexible in that specific > > case. > > We would create structures, fill them, convert them to JS, add them to map > > like and here we go. > > This would be no different than all the rest of IDL infrastructure. A c++ > backing, wrapped by a JavaScript object. In the RTCStatsReport specific case, the C++ structures (the dictionaries) would only last the time of the conversion. In the case of an attribute getter returning a dictionary, the corresponding C++ object will probably be kept, updated and manipulated. This is not the case here, we would just create the C++ structure for the purpose of obtaining the corresponding JS object. RTCStats are extensible, the WebIDL is not defining all fields provided by the WebRTC backend. We could beef up WebKit IDLs to cover all possible dictionaries. We would need to implement the marshalling of the WebRTC backend stats (a map) to the C++ structure which would be used to be converted to the dictionary. Currently, we just have to handle the type of each stat field. I do not see any advantage here. > > > > > Can you explain again why this can't be backed by a HashMap or if needs to > > > be ordered, some other map, where type conversions happen at the boundary? > > > > WebIDL is defining maplike exactly as implemented in this patch: an object > > exposing data stored into a backing JS map. > > To expose it properly as defined by WebIDL, we need to send back MapIterator > > objects to methods like values or entries. A MapIterator is bound to a JS > > Map. > > > > Is any of that actually user observable, or is the spec just using the > language of the ES spec to define the behavior. If for instance, > modifications to the Map prototype effect the behavior of maplike objects, > then it probably makes sense to implement it using the JS Map. But if not, I > would like to stick to our normal patterns. This is indeed user observable, maplike would be affected by Map/Map Iterator prototype changes.
<rdar://problem/30293780>
maplike and setlike being very close one from the other, I went trying to implement setlike in bug 159140. A similar design could be then applied to maplike.
Created attachment 301985 [details] Patch
Attachment 301985 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/JSDOMGuardedObject.h:60: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSDOMMapLike.h:65: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSDOMGlobalObject.h:37: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:121: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 4 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 301986 [details] Patch
Attachment 301986 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/JSDOMGuardedObject.h:60: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSDOMMapLike.h:65: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSDOMGlobalObject.h:37: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:121: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 4 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 301990 [details] Patch
Attachment 301990 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/JSDOMGuardedObject.h:60: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSDOMMapLike.h:65: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSDOMGlobalObject.h:37: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:121: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 4 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 302021 [details] Patch
Attachment 302021 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/JSDOMGuardedObject.h:60: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSDOMMapLike.h:66: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSDOMGlobalObject.h:37: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:121: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 4 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 302030 [details] Patch
Attachment 302030 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/JSDOMGuardedObject.h:60: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSDOMMapLike.h:66: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSDOMGlobalObject.h:37: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:121: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 4 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 302032 [details] Fixing GTK build
Attachment 302032 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/JSDOMGuardedObject.h:60: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSDOMMapLike.h:66: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSDOMGlobalObject.h:37: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:121: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 4 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 302032 [details] Fixing GTK build View in context: https://bugs.webkit.org/attachment.cgi?id=302032&action=review Let's write a test verifying the overwriting of maplike prototype so we can compare it with the behavior of other browsers and change the test if we change our minds. > Source/WebCore/Modules/mediastream/RTCStatsReport.h:41 > + extra space > Source/WebCore/bindings/scripts/test/TestMapLike.idl:26 > + maplike<DOMString, DOMString>; It would probably be a better test to have two different types to verify you got the right type in the right place.
Can you add tests that modify the Map prototype and show how it affects MapLike objects (for instance, replace the forEach function)?
Here is the related WebIDL thread: https://github.com/heycam/webidl/issues/254 The spec currently describes what the current patch is implementing. But the end goal is that maplike would be shielded from Map prototype changes. Hum, I think I would prefer that maplike functions do not return any MapIterator. That way, we could implement map like without any backing map.
Created attachment 302895 [details] Patch for landing
(In reply to comment #47) > Created attachment 302895 [details] > Patch for landing Patch does not include prototype change shielding. I'll do that as a follow-up and will add tests for it.
Comment on attachment 302895 [details] Patch for landing Clearing flags on attachment: 302895 Committed r213108: <http://trac.webkit.org/changeset/213108>
All reviewed patches have been landed. Closing bug.