1316277 - Consider removing mRegisteredIntersectionObservers from DOMSlots

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Olli Pettay [:smaug][[e-mail]]]

DOMSlots is becoming heavy. Is there a reason to use it for a rarely used feature like 
IntersectionObservers? Couldn't a global hashtable (similar to what is used for EventListenerManager for nodes) be used? Or perhaps properties (nsINode::Set/GetProperty)

Comment 1

[Tobias Schneider [:tobytailor]]

Attachment: Move intersection observer list from DOMSlots to a property

Comment 2

[Tobias Schneider [:tobytailor]]

Attachment: Move intersection observer list from DOMSlots to a property

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8911235 [details] [diff] [review]
Move intersection observer list from DOMSlots to a property

Review of attachment 8911235 [details] [diff] [review]:
-----------------------------------------------------------------

I have one comment to address that needs to happen before checkin. I'll stamp the next patch.

::: dom/base/Element.cpp
@@ +4152,5 @@
> +  if (!observers) {
> +    observers = new nsDataHashtable<nsRefPtrHashKey<DOMIntersectionObserver>,
> +                                                    int32_t>();
> +    SetProperty(nsGkAtoms::intersectionobserverlist, observers,
> +                nsINode::DeleteProperty<bool>);

This looks wrong. The template parameter to nsINode::DeleteProperty is the type of the pointer that the property points to. Here, we're not storing a bool, but rather a hashtable. It would probably help readability here to typedef `nsDataHashtable<nsRefPtrHashKey<DOMIntersectionObserver>, int32_t>` to something like IntersectionObserverList (maybe a private typedef on Element?) and use that.

Comment 4

[Tobias Schneider [:tobytailor]]

Attachment: Move intersection observer list from DOMSlots to a property

Addressed review comments.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8912356 [details] [diff] [review]
Move intersection observer list from DOMSlots to a property

Review of attachment 8912356 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 6

[Tobias Schneider [:tobytailor]]

Attachment: Move intersection observer list from DOMSlots to a property

Comment 7

[Tobias Schneider [:tobytailor]]

Comment on attachment 8915708 [details] [diff] [review]
Move intersection observer list from DOMSlots to a property

Blake, need you to give this another review since I had to make 2 important changes to the patch:

1. Previous patch accidentally removed slots->Unlink();, leading to memory leaks.
2. Setting the property if not defined should only happen if we actually need it. The previous patch always set the property, even if we only called RegisteredIntersectionObservers to check its presence.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8915708 [details] [diff] [review]
Move intersection observer list from DOMSlots to a property

Review of attachment 8915708 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for missing that stuff the last time.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce5016d6b5df
Move intersection observer list from DOMSlots to a property. r=mrbkap

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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