We already have enumProperties and enumEntries functions on the object actor. We should have a similar way to get an iterator for the symbols of a given grip.

Comment on attachment 8899866 [details] Bug 1392614 - Add an onEnumSymbols function on the object actor. https://reviewboard.mozilla.org/r/171160/#review176364 Looks good overall, thanks a lot for the test! But I would like you to confirm we have to fork PropertyIteratorActor/Client. See my suggestion about about to still share this code and tell me if you think that's worth. ::: devtools/server/actors/object.js:1154 (Diff revision 1) > + size: symbols.length, > + symbolDescription(index) { > + const symbol = symbols[index]; > + return { > + name: symbol.toString(), > + descriptor: objectActor._propertyDescriptor(symbol, true) I don't follow why you are forking PropertyIteratorActor. The main difference is that SymbolIteratorActor do not expose `names` request, and instead, return the names from slice/all (which PropertyIteratorActor does not). Shouldn't we have the same behavior between both? Have names method and slice/all only return value descriptor? If that's about the naming disturbing you, feel free to rename methods to make it more generic. Like PropertyIteratorActor => IteratorActor propertyDescription => itemDescription or description ownProperties => items For this last one, it is more complex as you would need compatiblity code to support multiple RDP versions. Note that you can add such compatibility code over here: http://searchfox.org/mozilla-central/source/devtools/shared/client/main.js#2826 via an `after` callback, like here: http://searchfox.org/mozilla-central/source/devtools/shared/client/main.js#2076 Where you can do: if ("ownProperties" in response) { response.items = reponse.ownProperties; } ::: devtools/server/actors/object.js:1182 (Diff revision 1) > + ownSymbols > + }; > + }, > + > + all() { > + return this.slice({ start: 0, count: this.length }); looks like this.length should be this.iterator.size ::: devtools/server/tests/unit/test_objectgrips-18.js:41 (Diff revision 1) > + let [grip] = packet.frame.arguments; > + > + let objClient = gThreadClient.pauseGrip(grip); > + > + // Checks the result of enumProperties. > + let response = await new Promise(resolve => objClient.enumProperties({}, resolve)); enumProperties and enumSymbols should already return a promise. ::: devtools/server/tests/unit/test_objectgrips-18.js:82 (Diff revision 1) > + const {iterator} = response; > + equal(iterator.count, 10, "iterator.count has the expected value"); > + > + do_print("Check iterator.slice response for all properties"); > + let sliceResponse = await new Promise(resolve => > + iterator.slice(0, iterator.count, resolve) iterator.slice should also return a promise.

Thanks for the review Alex > But I would like you to confirm we have to fork PropertyIteratorActor/Client. > See my suggestion about about to still share this code and tell me if you think that's worth. prototypeAndProperties returns an object which looks like : ``` { ownProperties: { "x" : {…}, "y" : {…}, }, ownSymbols: [ { name: "Symbol()" } ] } ``` I think this is why we have a similar structure when we call enumProperties : ``` { ownProperties: { "x" : {…}, "y" : {…}, } } ``` And I'd like us to keep the same consistency with ownSymbols, i.e. having : ``` { ownSymbols: [ { name: "Symbol()" } ] } ``` But if we use the PropertyIteratorActor, it will return us an object, where the "property name" will be the index of the symbol we get from Object.getOwnPropertySymbols, i.e. ``` { ownProperties: { "0" : { name: "Symbol()" } } } ``` We could change the name of the root property to "items", but the structure would still not really fit the concept of ownPropertySymbols (and won't be consistent with what we get from prototypeAndProperties). Also, it makes little sense to be able to get the "names" of this SymbolIterator, since we only have indeces, and the size can help us to decide if we want to slice them. At first I was re-using the PropertyIterator, and adding some properties in enumSymbols so PropertyIterator would act differently for symbols, but it felt a bit odd. What are the drawbacks you see in having a dedicated iterator for Symbols instead of using PropertyIterator ?

Comment on attachment 8899866 [details] Bug 1392614 - Add an onEnumSymbols function on the object actor. https://reviewboard.mozilla.org/r/171160/#review176364 > enumProperties and enumSymbols should already return a promise. I think it does, but then I don't have access to iterator.slice. I guess the Promise resolve before running the "after" callback (http://searchfox.org/mozilla-central/source/devtools/shared/client/main.js#2629-2632). I'll try to find where I can fix that.

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #3) > We could change the name of the root property to "items", but the structure > would still not really fit the concept of ownPropertySymbols (and won't be > consistent with what we get from prototypeAndProperties). I wasn't considering existing shapes from prototypeAndProperties. That's a good point. We should keep things similar between the two. But I'm wondering if the choices made in bug 881480 were the good one. prototypeAndProperties returns that: > { > ownSymbols: [ > { > name: "Symbol()" > descriptor: ... > } > ] > } It could have been in this form instead: > { > ownSymbols: { > "Symbol()": ... > } > } Then, things are much more similar between enumProperties and enumSymbols. > Also, it makes little sense to be able to get the "names" of this > SymbolIterator, since we only have indeces, and the size can help us to > decide if we want to slice them. You do have names. The symbol names. It is weird to handle property and symbol names differently. (in prototypeAndProperties as well as in enumProperties/enumSymbols) Isn't that also weird on the frontend side to be different between symbols and properties? > At first I was re-using the PropertyIterator, and adding some properties in > enumSymbols so PropertyIterator would act differently for symbols, but it > felt a bit odd. > > What are the drawbacks you see in having a dedicated iterator for Symbols > instead of using PropertyIterator ? The drawback is that it duplicates very similar code in actor and client, and the API is only slightly different, which can be confusing. If `{ ownSymbols: { "Symbol()": ... } }` makes sense to you, what about updating prototypeAndProperties to that new form and only tweak PropertyIteratorActor.slice to either put results in ownProperties or ownSymbols? If the frontend already uses prototypeAndProperties's ownSymbols against Firefox 56, you may have to add compatibility here: http://searchfox.org/mozilla-central/source/devtools/shared/client/main.js#2598-2605 getPrototypeAndProperties: DebuggerClient.requester({ type: "prototypeAndProperties" }, { after() { if (Array.isArray(response.ownSymbols)) { response.ownSymbols = convert array to object; } } }),

(In reply to Alexandre Poirot [:ochameau] from comment #5) > (In reply to Nicolas Chevobbe [:nchevobbe] from comment #3) > > We could change the name of the root property to "items", but the structure > > would still not really fit the concept of ownPropertySymbols (and won't be > > consistent with what we get from prototypeAndProperties). > > I wasn't considering existing shapes from prototypeAndProperties. That's a > good point. > We should keep things similar between the two. > > But I'm wondering if the choices made in bug 881480 were the good one. > prototypeAndProperties returns that: > > { > > ownSymbols: [ > > { > > name: "Symbol()" > > descriptor: ... > > } > > ] > > } > > It could have been in this form instead: > > > { > > ownSymbols: { > > "Symbol()": ... > > } > > } > I think we can't do that, because the following is perfectly valid and will give you 2 symbol properties : > const a = Symbol(); > const b = Symbol(); > const obj = { > [a]: "a", > [b]: "a", > }; > -> Object { Symbol(): "a", Symbol(): "a" } If we go with having names in place of indexes, we lose data here. We can't have > { > ownSymbols: { > "Symbol()": {…}, > "Symbol()": {…} > } > } This is why ownSymbols is an array and not a map-like object.

Comment on attachment 8899866 [details] Bug 1392614 - Add an onEnumSymbols function on the object actor. https://reviewboard.mozilla.org/r/171160/#review176698 Ok, your approach makes sense, thanks for the explanations! May be we could comment somewhere in object.js to explain why ownSymbols is an array and not an object? Do not hesitate to tweak iterator promises and the test in a followup.

Pushed by nchevobbe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e0d5b47b2d72 Add an onEnumSymbols function on the object actor. r=ochameau