Closed Bug 690414 Opened 8 years ago Closed 8 years ago

syncNotification.xml leaks 'Observers' and 'Notifications' into the global scope

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 10

People

(Reporter: dao, Assigned: dao)

Details

Attachments

(1 file)

Attached patch patchSplinter Review
No description provided.
Attachment #563449 - Flags: review?(gavin.sharp)
Comment on attachment 563449 [details] [diff] [review]
patch

>diff --git a/browser/base/content/syncNotification.xml b/browser/base/content/syncNotification.xml

>+        Cu.import("resource://services-sync/notifications.js", temp);
>+        for each (var notification in temp.Notifications.notifications)

Looks like this could use Weave.Notifications?
Attachment #563449 - Flags: review?(gavin.sharp) → review+
Possibly... Of course, there should be no global Weave object in the first place. (It should at least be called Sync.)
I don't think adding 1 more reference to it will significantly hinder efforts to rename it.
No, but I don't know that it needs to be in browser.js at all. I haven't looked into this lately.
https://hg.mozilla.org/mozilla-central/rev/e4ee17a3f362
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
Thanks for fixing this, Dão!

(In reply to Dão Gottwald [:dao] from comment #4)
> No, but I don't know that it needs to be in browser.js at all. I haven't
> looked into this lately.

browser-syncui.js uses it a lot. It's the main entry point for Sync's UI-facing APIs. Note that instead of importing the JSMs in the first place, the exposed objects could've been accessed via the 'Weave' object (Weave.Svc.Obs and Weave.Notifications).
You need to log in before you can comment on or make changes to this bug.