791163 - Add addon-options-displayed/addon-options-hidden as constants on AddonManager

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

The addon-options-displayed and addon-options-hidden notifications are used for inline settings in the UI. They're starting to be used by a couple of providers to add provider-specific UI (bug 335781, bug 619652). At the moment, they're just magic hard-coded values - since they're being re-used more (and also used in addons), they should become re-usable constants.

They should be named OPTIONS_NOTIFICATION_DISPLAYED and OPTIONS_NOTIFICATION_HIDDEN, and be added after the OPTIONS_TYPE_* constants on the AddonManager object in AddonManager.jsm:
https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/AddonManager.jsm


List of existing uses that should be updated to use the new constants:
https://mxr.mozilla.org/mozilla-central/search?string=addon-options-&regexp=1&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 1

[Julian]

Attachment: Partial Patch File Toolkit

Comment 2

[Julian]

Attachment: Partial Patch File Mobile

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 664086 [details] [diff] [review]
Partial Patch File Toolkit

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

I'll be extra pedantic, since you asked :)

When attaching a patch, you can (and should) request review from someone, so it doesn't get lost. In the attachment upload page, there are a series of flags. Set the review flag to "?", and enter the email of whoever you think should review the patch (can start entering a name, and it'll try to autocomplete). https://wiki.mozilla.org/Modules has a list of reviewers, or just look at whoever review code changes there recently (for the Add-ons Manager, its safe to assume its me).

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +2200,5 @@
>    // Options will be displayed in a new tab, if possible
>    OPTIONS_TYPE_TAB: 3,
>  
> +  // Constants for displayed or hidden options notifications
> +  // Options notification will be displayed 

Careful about accidentally inserting trailing whitespace.

::: toolkit/mozapps/extensions/content/extensions.js
@@ +2878,5 @@
>            gDetailView.node.addEventListener("ViewChanged", function viewChangedEventListener() {
>              gDetailView.node.removeEventListener("ViewChanged", viewChangedEventListener, false);
>              if (firstSetting)
>                firstSetting.clientTop;
> +            Services.obs.notifyObservers(document, AddonManager.OPTIONS_NOTIFICATION_DISPLAYED, gDetailView._addon.id);

Should try to wrap lines to 80 characters, and be sure to indent using spaces not tabs (see the other calls to addObserver in this patch).
See https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 664087 [details] [diff] [review]
Partial Patch File Mobile

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

Should fix the long lines here too.

Comment 5

[Julian]

Attachment: Partial Patch File Mobile

Comment 6

[Julian]

Attachment: Partial Patch File Toolkit

Comment 7

[Julian]

Attachment: Partial Patch File Mobile

Comment 8

[Julian]

Attachment: Partial Patch File Toolkit

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Landed on the fx-team branch, which will get merged into mozilla-central within a day or so:
https://hg.mozilla.org/integration/fx-team/rev/cd72779323e1

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/cd72779323e1