Closed Bug 721165 Opened 8 years ago Closed 7 years ago

Extract repeated code for retrieving CHANNEL information from Blocklist and Telemetry to a javascript module

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: mreid, Assigned: andreshm)

References

Details

(Whiteboard: [mentor=gavin][lang=js])

Attachments

(1 file)

There is duplicate code in 
toolkit/mozapps/extensions/nsBlocklistService.js
toolkit/components/telemetry/TelemetryPing.js
toolkit/components/urlformatter/nsURLFormatter.js

for "getUpdateChannel()" function and possibly elsewhere.  See bug 720785 for further discussion.
Component: Telemetry → General
QA Contact: telemetry → general
Whiteboard: [mentor=gavin][lang=js]
I'll take a look at this.
Assignee: nobody → andres
Attached patch Patch v1Splinter Review
Attachment #659758 - Flags: review?(gavin.sharp)
Attachment #659758 - Flags: review?(gavin.sharp) → review?(adw)
Comment on attachment 659758 [details] [diff] [review]
Patch v1

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

::: toolkit/content/UpdateChannel.jsm
@@ +13,5 @@
> +   * Read the update channel from defaults only.  We do this to ensure that
> +   * the channel is tightly coupled with the application and does not apply
> +   * to other instances of the application that may use the same profile.
> +   */
> +  get: function UpdateChannel_get() {

Please choose a more descriptive name for this method, like getDefaultUpdateChannel or getDefaultChannel.
Attachment #659758 - Flags: review?(adw) → review+
(In reply to Drew Willcoxon :adw from comment #3)
> > +  get: function UpdateChannel_get() {
> 
> Please choose a more descriptive name for this method, like
> getDefaultUpdateChannel or getDefaultChannel.

Isn't UpdateChannel.getDefaultUpdateChannel() redundant? UpdateChannel.get() seems pretty clear self-explanatory.
I agree that is clear UpdateChannel.get(), but if in the future there can be more getters then maybe we can use UpdateChannel.getDefault()?

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=17d15b1c5f74
I don't think there will be a need for future additional getters, this module is very simple.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/6205f156d432
Status: NEW → ASSIGNED
Keywords: checkin-needed
Whiteboard: [mentor=gavin][lang=js] → [mentor=gavin][lang=js][fixed-in-fx-team]
Version: 12 Branch → Trunk
Attachment #659758 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6205f156d432
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=gavin][lang=js][fixed-in-fx-team] → [mentor=gavin][lang=js]
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.