1887472 - SetLikeHelpers::Add should return if it succeeded or not

Status: RESOLVED WONTFIX

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

Description

[Keith Cirkel [:keithamus]]

Steps to reproduce:

See related https://phabricator.services.mozilla.com/D205518.

SetLikeHelpers::Add currently returns void but could return a bool to determine if set has changed.

Comment 1

[Monica Chiorean]

Keith, can this be marked as new (instead of unconfirmed) since it is an enhancement. Thank you.

Comment 2

[Simona Badau, Desktop QA]

Moving this over to a potential component as I am not sure where this would fit best. If this is not the right, please change to a more suitable one.
Thanks!

Comment 3

[Edgar Chen [:edgar]]

SetLikeHelpers::Add should throw an error to ErrorRequest if the insertion is not success per https://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/ElementInternalsBinding.cpp#491-494, no? Or maybe I miss something.

Comment 4

[Jan Jaeschke [:jjaschke]]

In fact, this would actually simplify the code in Highlight::Add(). In this specific case, I'm not sure I would want Highlight::Add() to return an error. Adding an existing element to a set should be a noop, not throw an error.

Comment 5

[Jan Jaeschke [:jjaschke]]

Hi Keith,

Edgar and I took a short look at this issue and your use case.

First of all, regarding your use case: I had a similar issue in Highlight::Add() mentioned in the comment above this one. The pragmatic fix for this would be to have your ::Add() function check if the element exists using SetlikeHelpers::Has() before adding the element. Theoretically this imposes a performance overhead due to having another lookup, but in most cases this should be negligible. In my case, I have a mirror C++ data structure for the content of the setlike, which is an nsTArray<> to preserve ordering. Originally I used nsTArray<>::Contains() as well to check if the element is already contained in the set, which proved to be too slow. Changing this to use ::Has() drastically improved this, see Bug 1892589.

As for changing the return value of SetlikeHelpers::Add(), we did look at add()/insert() operations for sets in various languages to see if the function returns an indicator whether an element has been added. There is no consistent standard way of indicating if an element has been added across languages:

• Python returns None
• JS returns the object (to allow chaining)
• C++ returns a pair of iterator to the added element and "has been added" bool
• Rust returns a "has been added" bool

The JS API that is being called from the binding layer does not indicate whether the element already exists in the set. If it would, there would be an exception (if you follow along in the link Edgar posted, you can see that all error cases result in a false return value and an exception on JS side. So we can't use that).
This means that likely the solution for this issue would be to add the Has() call in the binding layer. However, this would have a (albeit very small and in most cases not noticeable) performance impact, which is why I don't think we should do this.

Does this sound reasonable to you? :)

Comment 6

[Keith Cirkel [:keithamus]]

I think this seems reasonable. In my case I also need to maintain a mirror structure so I can kick off CSS invalidation for each added/removed state so it's not that burdensome to keep checks in Add.

Given this, should I close this as WONTFIX?

Comment 7

[Jan Jaeschke [:jjaschke]]

Yes, let's close this as WONTFIX.

Btw., at some point I'll attempt to get rid of these mirrored arrays by providing iterator access to the maplike/setlike data, see Bug 1852908. Would this be something you could benefit from as well?

Comment 8

[Keith Cirkel [:keithamus]]

Yes that sounds very helpful!

Comment 9

[Jan Jaeschke [:jjaschke]]

Noted :)