Closed Bug 1181930 Opened 4 years ago Closed 4 years ago

Refactoring: move the message broadcaster out of Webapps.jsm

Categories

(Core Graveyard :: DOM: Apps, defect)

defect
Not set

Tracking

(firefox42 fixed)

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: fabrice, Assigned: fabrice)

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Attached patch apps-broadcaster.patch (obsolete) — Splinter Review
That's a small and safe refactoring, but I need it for another patch I'm working on.
Assignee: nobody → fabrice
Attachment #8631404 - Flags: review?(ferjmoreno)
Attached patch apps-broadcaster.patch (obsolete) — Splinter Review
With the new module actually added to the patch...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e9556029e34
Attachment #8631404 - Attachment is obsolete: true
Attachment #8631404 - Flags: review?(ferjmoreno)
Attachment #8631408 - Flags: review?(ferjmoreno)
Attachment #8631408 - Flags: review?(ferjmoreno)
Now passing tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fae526a85600
Attachment #8631408 - Attachment is obsolete: true
Attachment #8639891 - Flags: review?(ferjmoreno)
Comment on attachment 8639891 [details] [diff] [review]
apps-broadcaster.patch

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

Nice!

::: dom/apps/MessageBroadcaster.jsm
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +const { utils: Cu, classes: Cc, interface: Ci } = Components;

None of these constants are used.

@@ +12,5 @@
> +this.EXPORTED_SYMBOLS = ["MessageBroadcaster"];
> +
> +this.MessageBroadcaster = {
> +  appGetter: null,
> +  children: [],

Maybe "childrens"

@@ +21,5 @@
> +    }
> +    this.appGetter = aAppGetter;
> +  },
> +
> +  addMessageListener: function(aMsgNames, aApp, aMm) {

Maybe add a comment explaining that listeners are ref counted.

@@ +22,5 @@
> +    this.appGetter = aAppGetter;
> +  },
> +
> +  addMessageListener: function(aMsgNames, aApp, aMm) {
> +    aMsgNames.forEach(function (aMsgName) {

Use arrow functions here and in the rest of the patch, please.

@@ +23,5 @@
> +  },
> +
> +  addMessageListener: function(aMsgNames, aApp, aMm) {
> +    aMsgNames.forEach(function (aMsgName) {
> +      let man = aApp && aApp.manifestURL;

s/man/manifestURL/g, please

@@ +46,5 @@
> +
> +      // If the state reported by the registration is outdated, update it now.
> +      if ((aMsgName === 'Webapps:FireEvent') ||
> +          (aMsgName === 'Webapps:UpdateState')) {
> +        if (man) {

if (man && ((aMsgName === 'Webapps:FireEvent') || (aMsgName === 'Webapps:UpdateState')))
Attachment #8639891 - Flags: review?(ferjmoreno) → review+
https://hg.mozilla.org/mozilla-central/rev/939e1009ebaf
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.