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

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Tobias Schneider [:tobytailor]]

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.

Comment 1

[Jet Villegas (inactive)]

(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?"

Comment 2

[Tobias Schneider [:tobytailor]]

Link to specific part of the spec: https://wicg.github.io/IntersectionObserver/#update-intersection-observations-algo, point 2.

Comment 3

[Tobias Schneider [:tobytailor]]

Attachment: Send update notifications in same order as observed

Use nsCOMArray instead of nsHashtable for mObservationTargets to guarantee insertion-order when iterating over its keys.

Comment 4

[Tobias Schneider [:tobytailor]]

Attachment: Send update notifications in same order as observed

Comment 5

[Tobias Schneider [:tobytailor]]

Attachment: Send update notifications in same order as observed

Rebased patch.

Comment 6

[Jet Villegas (inactive)]

Needs a test. Also needs a careful review of the object lifetime effects of this change.

Comment 7

[Tobias Schneider [:tobytailor]]

See See https://bugzilla.mozilla.org/show_bug.cgi?id=1359318#c8.

Comment 8

[Tobias Schneider [:tobytailor]]

Attachment: Send update notifications in same order as observed

Don't block enabling web platform test.

Comment 9

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

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.

Comment 10

[Tobias Schneider [:tobytailor]]

Attachment: Send update notifications in same order as observed

Addressed review comments.

Comment 11

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

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 *

Comment 12

[Tobias Schneider [:tobytailor]]

Attachment: Send update notifications in same order as observed. r=smaug

Nits.

Comment 13

[Eric Shepherd [:sheppy]]

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).

Comment 14

[Jet Villegas (inactive)]

(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.

Comment 15

[Tobias Schneider [:tobytailor]]

Attachment: Send update notifications in same order as observed. r=smaug

Rebased patch.

Comment 16

[Tobias Schneider [:tobytailor]]

Attachment: Send update notifications in same order as observed

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.

Comment 17

[Tobias Schneider [:tobytailor]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=839fe6214ca56650a335ced60d77c3c49cf2fb4a

Comment 18

[Tobias Schneider [:tobytailor]]

Another try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=891b402ce8c26f6b2d40b196a3d6fcc93f060536

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c285e406afb2
(intersection-observer) Send update notifications in same order as observed. r=smaug

Comment 20

[Carsten Book [:Tomcat]]

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

Comment 21

[Eric Shepherd [:sheppy]]

Removed DDN since this will be in place for initial availability.