Closed Bug 1359311 Opened 3 years ago Closed 2 years ago

(intersection-observer) Send update notifications in same order as observed

Categories

(Core :: Layout, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: tschneider, Assigned: tschneider)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 7 obsolete files)

At the moment we use a nsHashtable to keep a list of observations. When iterating over its keys, it doesn't guarantee to enumerate in the same order as they were inserted. The Intersection Observer spec mentions that this should be the case.
Blocks: 1358666
(In reply to Tobias Schneider [:tobytailor] from comment #0)
> At the moment we use a nsHashtable to keep a list of observations. When
> iterating over its keys, it doesn't guarantee to enumerate in the same order
> as they were inserted. The Intersection Observer spec mentions that this
> should be the case.

Direct spec link? Also "Notificate?"
Summary: (intersection-observer) Notificate in same order as observed → (intersection-observer) Send update notifications in same order as observed
Use nsCOMArray instead of nsHashtable for mObservationTargets to guarantee insertion-order when iterating over its keys.
Attachment #8862939 - Attachment is obsolete: true
Rebased patch.
Attachment #8862947 - Attachment is obsolete: true
Needs a test. Also needs a careful review of the object lifetime effects of this change.
Don't block enabling web platform test.
Attachment #8863195 - Attachment is obsolete: true
Attachment #8868635 - Flags: review?(bugs)
No longer blocks: 1358666
Depends on: 1358666
Comment on attachment 8868635 [details] [diff] [review]
Send update notifications in same order as observed

The old mObservationTargets keeps raw pointers to elements, new one has strong refs. Aren't we leaking.

Don't you want nsTArray<Element*>, and a comment that the raw pointers are explicitly cleared by UnlinkTarget calls.
Attachment #8868635 - Flags: review?(bugs) → review-
Addressed review comments.
Attachment #8868635 - Attachment is obsolete: true
Attachment #8868671 - Flags: review?(bugs)
Comment on attachment 8868671 [details] [diff] [review]
Send update notifications in same order as observed

> DOMIntersectionObserver::UnlinkTarget(Element& aTarget)
> {
>     if (!mObservationTargets.Contains(&aTarget)) {
>         return;
>     }
Could you now remove this. Contains is O(n) and so is RemoveElement, so at least we could
remove one possibly slow call.

> 
>-    mObservationTargets.RemoveEntry(&aTarget);
>-    if (mObservationTargets.Count() == 0) {
>+    mObservationTargets.RemoveElement(&aTarget);
>+    if (mObservationTargets.Length() == 0) {
>         Disconnect();
Not about this issue, but Disconnect() is intended using 4 spaces, should be just 2

>+  
>+  // Holds raw pointers which are explicitly cleared by UnlinkTarget().
>+  nsTArray<Element *>                             mObservationTargets;
Nit, extra space before *
Attachment #8868671 - Flags: review?(bugs) → review+
Nits.
Attachment #8868671 - Attachment is obsolete: true
Adding dev-doc-needed so I can be sure this fix is reflected in docs if by some chance it's not in initial release (which I doubt will be the case, but at this point I'm documenting this API so I need to be sure).
Keywords: dev-doc-needed
(In reply to Eric Shepherd [:sheppy] from comment #13)
> Adding dev-doc-needed so I can be sure this fix is reflected in docs if by
> some chance it's not in initial release (which I doubt will be the case, but
> at this point I'm documenting this API so I need to be sure).

Of the IntersectionObserver fixes proposed to fix Web Platform Tests (see the 'blocks' list in bug 1358666,) this one is riskier to product stability. We may hold this change back one release to get more testing on it.
Blocks: 1374355
Rebased patch.
Attachment #8868713 - Attachment is obsolete: true
Olli, I had to make small changes to the patch already reviewed by you. Latest builds failed due to incompatible types used for loop variables. Had to change it from uint32_t to size_t. Please give it a quick review.
Attachment #8880580 - Attachment is obsolete: true
Attachment #8880707 - Flags: review?(bugs)
Attachment #8880707 - Flags: review?(bugs) → review+
Pushed by tschneider@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c285e406afb2
(intersection-observer) Send update notifications in same order as observed. r=smaug
https://hg.mozilla.org/mozilla-central/rev/c285e406afb2
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Blocks: 1381574
Removed DDN since this will be in place for initial availability.
Keywords: dev-doc-needed
Assignee: nobody → tschneider
You need to log in before you can comment on or make changes to this bug.