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

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

Comment 1

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 2

[Dão Gottwald [:dao]]

Possibly... Of course, there should be no global Weave object in the first place. (It should at least be called Sync.)

Comment 3

[:Gavin Sharp [email: [e-mail]]]

I don't think adding 1 more reference to it will significantly hinder efforts to rename it.

Comment 4

[Dão Gottwald [:dao]]

No, but I don't know that it needs to be in browser.js at all. I haven't looked into this lately.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 6

[Philipp von Weitershausen [:philikon]]

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).