Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

Merge v10 #3191

Merged
merged 16 commits into from
Jul 24, 2017
Merged

Merge v10 #3191

merged 16 commits into from
Jul 24, 2017

Conversation

ianb
Copy link
Contributor

@ianb ianb commented Jul 21, 2017

Fixes #3142

The ui.js merge was a little hairy, extra attention to that file would be welcome.

johngruen and others added 16 commits July 10, 2017 17:06
This adds : (important on Windows), \, <, and > to the blacklist.
Followup in #3083
In test conditions, when the browser is started and stopped very quickly, sometimes we didn't shut down the WebExtension because it hadn't fully started.
r=kmag (in https://bugzilla.mozilla.org/show_bug.cgi?id=1373614)
* Validate iframe URLs
Remove unneeded iframe onload handlers

* Put temporary clipboard TEXTAREA in an iframe
With iframe URL validation
* set dimensions for icon and add to startup (#3136)

* Address 10.6 review comments (bug 1381132)

* Update version to 10.7.0 with changelog
* Do not re-wrap onResize when adding and removing the listener
This caused the removeEventListener to silently fail, as the wrapped functions did not match (by identity)
Fixes #3153

* Suppress error popups for all errors in resize handlers
Errors still go to Sentry
Copy link
Member

@jaredhirsch jaredhirsch left a comment

Choose a reason for hiding this comment

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

R+. One minor memory leak that can be fixed now or in a followup bug, your call. Feel free to self-merge.

@@ -275,14 +277,16 @@ this.ui = (function() { // eslint-disable-line no-unused-vars
},

hide() {
window.removeEventListener("scroll", this.onScroll);
window.removeEventListener("scroll", watchFunction(assertIsTrusted(this.onScroll)));
Copy link
Member

Choose a reason for hiding this comment

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

I didn't catch this before, but just as with this.onResize, if we wrap this.onScroll inline, removeEventListener will fail to detach the listener. It doesn't lead to any thrown errors, but is a memory leak. I'm fine to address this here or in a followup bug.

Copy link
Contributor Author

@ianb ianb Jul 24, 2017

Choose a reason for hiding this comment

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

I'll make a followup: #3198

@jaredhirsch jaredhirsch merged commit 28df958 into master Jul 24, 2017
@jaredhirsch jaredhirsch deleted the merge-v10 branch July 24, 2017 16:33
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants