Inline prefs should send a notification when they stop displaying

RESOLVED FIXED in mozilla13

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Unfocused, Assigned: darktrojan)

Tracking

({dev-doc-complete})

Trunk
mozilla13
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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

Comment 1

6 years ago
Created attachment 603666 [details] [diff] [review]
patch
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-
(Assignee)

Comment 3

6 years ago
Created attachment 603918 [details] [diff] [review]
patch v2
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+
(Assignee)

Comment 5

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/32ebee0c8e80
Flags: in-testsuite+
Flags: in-litmus-
Target Milestone: --- → mozilla13
(Assignee)

Comment 6

6 years ago
I've mentioned the new notification here:
https://developer.mozilla.org/en/Extensions/Inline_Options
Keywords: dev-doc-complete
https://hg.mozilla.org/mozilla-central/rev/32ebee0c8e80
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.