Last Comment Bug 791163 - Add addon-options-displayed/addon-options-hidden as constants on AddonManager
: Add addon-options-displayed/addon-options-hidden as constants on AddonManager
Status: RESOLVED FIXED
[good first bug][mentor=bmcbride@mozi...
: dev-doc-needed
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla18
Assigned To: Julian
:
:
Mentors:
Depends on:
Blocks: 335781 619652
  Show dependency treegraph
 
Reported: 2012-09-13 21:47 PDT by Blair McBride [:Unfocused] (UNAVAILABLE)
Modified: 2012-10-02 01:02 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Partial Patch File Toolkit (8.25 KB, patch)
2012-09-24 08:58 PDT, Julian
blair: review-
Details | Diff | Splinter Review
Partial Patch File Mobile (2.10 KB, patch)
2012-09-24 08:59 PDT, Julian
blair: review-
Details | Diff | Splinter Review
Partial Patch File Mobile (2.12 KB, patch)
2012-09-27 05:36 PDT, Julian
no flags Details | Diff | Splinter Review
Partial Patch File Toolkit (8.67 KB, patch)
2012-09-27 05:39 PDT, Julian
no flags Details | Diff | Splinter Review
Partial Patch File Mobile (2.25 KB, patch)
2012-09-27 07:41 PDT, Julian
blair: review+
Details | Diff | Splinter Review
Partial Patch File Toolkit (8.60 KB, patch)
2012-09-27 07:42 PDT, Julian
blair: review+
Details | Diff | Splinter Review

Description Blair McBride [:Unfocused] (UNAVAILABLE) 2012-09-13 21:47:52 PDT
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 2012-09-24 08:58:49 PDT
Created attachment 664086 [details] [diff] [review]
Partial Patch File Toolkit
Comment 2 Julian 2012-09-24 08:59:36 PDT
Created attachment 664087 [details] [diff] [review]
Partial Patch File Mobile
Comment 3 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-09-24 21:18:31 PDT
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) 2012-09-24 21:20:52 PDT
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 2012-09-27 05:36:29 PDT
Created attachment 665371 [details] [diff] [review]
Partial Patch File Mobile
Comment 6 Julian 2012-09-27 05:39:18 PDT
Created attachment 665374 [details] [diff] [review]
Partial Patch File Toolkit
Comment 7 Julian 2012-09-27 07:41:55 PDT
Created attachment 665447 [details] [diff] [review]
Partial Patch File Mobile
Comment 8 Julian 2012-09-27 07:42:31 PDT
Created attachment 665448 [details] [diff] [review]
Partial Patch File Toolkit
Comment 9 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-09-30 19:11:19 PDT
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] 2012-10-02 01:02:36 PDT
https://hg.mozilla.org/mozilla-central/rev/cd72779323e1

Note You need to log in before you can comment on or make changes to this bug.