1392614 - Add an enumSymbols function on the objectActor to enable lazy loading

Status: RESOLVED FIXED

(DevTools :: Console, enhancement)

Description

[Nicolas Chevobbe [:nchevobbe]]

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 1

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1392614 - Add an onEnumSymbols function on the object actor.

This will enable lazy loading symbols for a grip if needed.
This patch introduce a new SymbolIteratorActor, which is similar
to PropertyIteratorActor but with a data structure that better
fits symbols, and how we already handle them, i.e. an `ownSymbols`
array property (and not an `ownProperties` object like PropertyIteratorActor).

We take this as an opportunity to add a test for enumSymbols, but also
for enumProperties that did not have server unit tests (it is only tested
in the frontend with the variable view, which might be deprecated at some
point).

Review commit: https://reviewboard.mozilla.org/r/171160/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/171160/

Comment 2

[Alexandre Poirot [:ochameau]]

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.

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

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 4

[Nicolas Chevobbe [:nchevobbe]]

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.

Comment 5

[Alexandre Poirot [:ochameau]]

(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; }
    }
  }),

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

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

[Alexandre Poirot [:ochameau]]

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.

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8899866 [details]
Bug 1392614 - Add an onEnumSymbols function on the object actor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171160/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e0d5b47b2d72
Add an onEnumSymbols function on the object actor. r=ochameau

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e0d5b47b2d72