Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #4246: RTL text plugin caused leak on Style#_remove() #4248

Merged
merged 2 commits into from
Feb 27, 2017

Conversation

ChrisLoer
Copy link
Contributor

@ChrisLoer ChrisLoer commented Feb 10, 2017

The unit test verifies that the "deregister" call happens, but because actually exercising the plugin-loading code depends on the network, I fell back to manual testing.

I tested by creating two maps, verifying there were two callbacks registered, removing one map and seeing that the second callback was removed, then calling setRTLTextPlugin to verify the remaining callback was successfully called and then discarded.

@jfirebaugh @mourner

@lucaswoj
Copy link
Contributor

This is starting to feel like an implementation of Evented. Would it be possible to use Evented to manage plugin available callbacks? That code is well tested and believed to be memory-leak free.

@ChrisLoer
Copy link
Contributor Author

@lucaswoj I switched to usingEvented, but had to modify the implementation Evented.once() so that off() works on one-time listeners as well as listeners added with on().

I also modified Evented.listens() so that it wouldn't return true when listeners had been added and then removed. The rest of this PR doesn't depend on that change, but at least based on my reading of the function documentation, it's the expected behavior.

 - Evented.once() listeners can now be removed with off()
 - Evented.listens() now returns false if all listeners removed
Copy link
Contributor

@lucaswoj lucaswoj left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for the bonus bugfix and extensive test cases.

@ChrisLoer ChrisLoer merged commit 171db61 into master Feb 27, 2017
@ChrisLoer ChrisLoer deleted the cloer_regression_4246 branch February 27, 2017 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants