733677 - Inline prefs should send a notification when they stop displaying

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: patch

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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?

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: patch v2

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 5

[Geoff Lankow (:darktrojan)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/32ebee0c8e80

Comment 6

[Geoff Lankow (:darktrojan)]

I've mentioned the new notification here:
https://developer.mozilla.org/en/Extensions/Inline_Options

Comment 7

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/32ebee0c8e80