Closed Bug 1316277 Opened 3 years ago Closed 2 years ago

Consider removing mRegisteredIntersectionObservers from DOMSlots

Categories

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

50 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: smaug, Assigned: tschneider)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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)
Priority: -- → P3
Blocks: 1381574
Assignee: nobody → tschneider
Blocks: 1399603
Attachment #8910459 - Attachment is obsolete: true
Attachment #8911235 - Flags: review?(mrbkap)
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.
Attachment #8911235 - Flags: review?(mrbkap) → review-
Addressed review comments.
Attachment #8911235 - Attachment is obsolete: true
Attachment #8912356 - Flags: review?(mrbkap)
Comment on attachment 8912356 [details] [diff] [review]
Move intersection observer list from DOMSlots to a property

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

Thanks!
Attachment #8912356 - Flags: review?(mrbkap) → review+
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.
Attachment #8915708 - Flags: review?(mrbkap)
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.
Attachment #8915708 - Flags: review?(mrbkap) → review+
Pushed by tschneider@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce5016d6b5df
Move intersection observer list from DOMSlots to a property. r=mrbkap
https://hg.mozilla.org/mozilla-central/rev/ce5016d6b5df
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1416450
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.