Skip to content
This repository has been archived by the owner on Mar 13, 2018. It is now read-only.

fixes #504, avoids global list of all MutationObservers #505

Merged
merged 1 commit into from
Sep 9, 2014

Conversation

jmesserly
Copy link
Contributor

They're now added to the global list on-demand, and it is sorted by ID before delivering. Sounds like this is similar to the Blink implementation (https://github.com/Polymer/ShadowDOM/issues/504#issuecomment-55013158)

@jmesserly
Copy link
Contributor Author

@arv mind taking a look?

@@ -180,13 +183,11 @@
record.oldValue = associatedStrings[uid];

// 8.
if (!observer.records_.length) {
scheduleCallback(observer);
}
observer.records_.push(record);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this reordering intentional?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind.

@arv
Copy link
Contributor

arv commented Sep 9, 2014

LGTM

Nice work around

@arv
Copy link
Contributor

arv commented Sep 9, 2014

Actually. Could we move the call to scheduleCallback out of the loop. No need to do that work more than once.

@jmesserly
Copy link
Contributor Author

Actually. Could we move the call to scheduleCallback out of the loop. No need to do that work more than once.

Done.

Yeah was thinking it probably didn't matter much perf wise (because of the length check, and the isScheduled check inside scheduleCallback), and I liked having one less state variable and slightly better "encapsulation" of the global list (only schedule/notify touch it). But those are very minor things in the grand scheme of things. :)

@jmesserly
Copy link
Contributor Author

by the way, @arv do you usually prefer to squash into a single commit before submitting a pull req?

@arv
Copy link
Contributor

arv commented Sep 9, 2014

LGTM

Squashing is preferred. Less churn.

@jmesserly
Copy link
Contributor Author

thanks! will squash the follow up & land :)

jmesserly pushed a commit that referenced this pull request Sep 9, 2014
fixes #504, avoids global list of all MutationObservers
@jmesserly jmesserly merged commit 740d3d3 into master Sep 9, 2014
@jmesserly jmesserly deleted the observer_leak branch September 9, 2014 22:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants