Closed Bug 733677 Opened 8 years ago Closed 8 years ago

Inline prefs should send a notification when they stop displaying

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: Unfocused, Assigned: darktrojan)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

When inline prefs are shown, the UI sends a addon-options-displayed notification. Extensions can use this to register DOM event listeners. However, they're not notified when they should remove those listeners, which will lead to leaks.

(Well, they could listen for the ViewChanged event, but that seems disconnected and not optimal.)
Attached patch patch (obsolete) — Splinter Review
Attachment #603666 - Flags: review?(bmcbride)
Comment on attachment 603666 [details] [diff] [review]
patch

Review of attachment 603666 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/content/extensions.js
@@ +2756,5 @@
>      AddonManager.removeManagerListener(this);
>      this.clearLoading();
>      if (this._addon) {
> +      if (this._addon.optionsType == AddonManager.OPTIONS_TYPE_INLINE)
> +        Services.obs.notifyObservers(document, "addon-options-hidden", null);

Consumers of this notification should get the addon ID, just like the displayed notification.

@@ +2952,5 @@
>  
>    onDisabling: function() {
>      this.updateState();
> +    if (this._addon.optionsType == AddonManager.OPTIONS_TYPE_INLINE)
> +      Services.obs.notifyObservers(document, "addon-options-hidden", null);

The settings are hidden on onDisabled, which will only happen for restartless extensions - will need to check the first parameter given to onDisabling (aNeedsRestart) to see if onDisabled will ever fire.

::: toolkit/mozapps/extensions/test/browser/browser_inlinesettings.js
@@ +22,5 @@
> +  checkNotDisplayed: function() {
> +    is(this.lastDisplayed, null, "'addon-options-displayed' notification should not have fired");
> +  },
> +  hiddenNotified: false,
> +  checkHidden: function() {

Can you add a checkNotHidden() too?
Attachment #603666 - Flags: review?(bmcbride) → review-
Attached patch patch v2Splinter Review
Attachment #603666 - Attachment is obsolete: true
Attachment #603918 - Flags: review?(bmcbride)
Comment on attachment 603918 [details] [diff] [review]
patch v2

Review of attachment 603918 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/content/extensions.js
@@ +2756,5 @@
>      AddonManager.removeManagerListener(this);
>      this.clearLoading();
>      if (this._addon) {
> +      if (this._addon.optionsType == AddonManager.OPTIONS_TYPE_INLINE)
> +        Services.obs.notifyObservers(document, "addon-options-hidden", this._addon.id);

Style nit: Wrap to 80 characters.

@@ +2954,3 @@
>      this.updateState();
> +    if (!aNeedsRestart && this._addon.optionsType == AddonManager.OPTIONS_TYPE_INLINE)
> +      Services.obs.notifyObservers(document, "addon-options-hidden", this._addon.id);

Style nit: Wrap to 80 characters.
Attachment #603918 - Flags: review?(bmcbride) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/32ebee0c8e80
Flags: in-testsuite+
Flags: in-litmus-
Target Milestone: --- → mozilla13
I've mentioned the new notification here:
https://developer.mozilla.org/en/Extensions/Inline_Options
https://hg.mozilla.org/mozilla-central/rev/32ebee0c8e80
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.