1891784 - Support HTML reflected attributes returning FrozenArray

Status: RESOLVED FIXED

(Core :: DOM: Bindings (WebIDL), defect)

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: Bug 1891784 - Support HTML reflected attributes returning FrozenArray. r?edgar!

Comment 2

[Peter Van der Beken [:peterv]]

Eitan, could you try this patch?

Your attribute should look like:

  [Frozen, ReflectedHTMLAttributeReturningFrozenArray] attribute sequence<Element>? ariaFoo;

and the C++ getter:

void GetAriaFoo(bool& aUseCachedValue, Nullable<nsTArray<RefPtr<Element>>>& aResult);

aUseCachedValue should be set to "the contents of elements is equal to the contents of this's cached attr-associated elements", see step 2 in the getter steps (https://html.spec.whatwg.org/#reflecting-content-attributes-in-idl-attributes:cached-attr-associated-elements). If it's true you should be able to return immediately without touching aResult. If it's false then you should do some of the other getter steps (not the conversion to and caching of the FrozenArray, that happens in the binding code), and set aResult to elements (or cached attr-associated elements, which should be equal after step 4).

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: Diff of the generated binding code

Comment 4

[Eitan Isaacson [:eeejay]]

That definitely looks like something I can work with. I'll give it a try. How should we resolve the issue of there being strong references to elements cached in the slot? Will our cycle collector handle that correctly?

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: Diff of the generated binding code

Comment 6

[Eitan Isaacson [:eeejay]]

The one thing I noticed so far that makes this not usable is the fact that the getter doesn't know if this is actually cached yet in the js slot, or has been cleared (eg. by the setter).

To indicate the existence of a cached array, I think you would either need aUseCachedValue to have an initial value of true when the getter is called, or even have aResult be prepolulated with the members of the cached array. That way you can do a direct comparison.

Comment 7

[Peter Van der Beken [:peterv]]

(In reply to Eitan Isaacson [:eeejay] from comment #4)

How should we resolve the issue of there being strong references to elements cached in the slot? Will our cycle collector handle that correctly?

That's unavoidable given how the spec is written. And yes, it should be cleaned up eventually by CC/GC.

(In reply to Eitan Isaacson [:eeejay] from comment #6)

The one thing I noticed so far that makes this not usable is the fact that the getter doesn't know if this is actually cached yet in the js slot, or has been cleared (eg. by the setter).

It should be irrelevant for your code if there is a cached array in the slot. You can't use the cached array that's in the slot anyway, that's a JS array containing JS values. This is exactly the reason why the spec has "cached attr-associated elements" and a "cached attr-associated elements object". The slot contains the "cached attr-associated elements object", you need to hold the "cached attr-associated elements" in the C++ object. Every time the getter is called you need to "get the attr-associated elements" and compare that to the "cached attr-associated elements", this is all in the getter steps in the spec. It does mean storing two arrays with references to the same elements (one converted to JS), but that's how it is. It's the reason why we think this really ought to be an ObservableArray (see https://github.com/whatwg/html/issues/10246).

The setter doesn't clear, see the setter steps in the spec. It sets the "explicitly set attr-elements", which is yet another array of elements. I recommend carefully going through the spec, there's a lot of subtleties to get right.

Comment 8

[Eitan Isaacson [:eeejay]]

(In reply to Peter Van der Beken [:peterv] from comment #7)

It should be irrelevant for your code if there is a cached array in the slot. You can't use the cached array that's in the slot anyway, that's a JS array containing JS values. This is exactly the reason why the spec has "cached attr-associated elements" and a "cached attr-associated elements object". The slot contains the "cached attr-associated elements object", you need to hold the "cached attr-associated elements" in the C++ object.

I was hoping the "cached attr-associated elements object", which I assume is the slotted object can be used to construct the "cached attr-associated elements" that we can compare against, and avoid the need for a second array in C++. But you are saying that it is opaque?

It's the reason why we think this really ought to be an ObservableArray (see https://github.com/whatwg/html/issues/10246).

I wasn't aware that this is being reconsidered. Should we just "lead the charge", as Anne says, and just skip a flawed FrozenArray altogether?

Comment 9

[Peter Van der Beken [:peterv]]

(In reply to Eitan Isaacson [:eeejay] from comment #8)

I was hoping the "cached attr-associated elements object", which I assume is the slotted object can be used to construct the "cached attr-associated elements" that we can compare against, and avoid the need for a second array in C++. But you are saying that it is opaque?

I think you should assume it is. There are ways we could reconstruct the "cached attr-associated elements", but it's way more painful than caching that array in the C++ object. In essence, the C++ getter should implement steps 1, 2 and 4 and the bindings code does 3, 5 and 6.

I wasn't aware that this is being reconsidered. Should we just "lead the charge", as Anne says, and just skip a flawed FrozenArray altogether?

That requires quite a bit of HTML spec work that I'm not willing to sign up for myself right now. So it depends on how much you want to postpone this. Note that this approach requires monitoring the DOM for id changes, which IIRC you wanted to avoid.

Comment 10

[Eitan Isaacson [:eeejay]]

The current patch works well for this, I have all relevant wpt tests passing locally.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0eb53e757c68
Support HTML reflected attributes returning FrozenArray. r=edgar

Comment 12

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/0eb53e757c68