Last Comment Bug 690414 - syncNotification.xml leaks 'Observers' and 'Notifications' into the global scope
: syncNotification.xml leaks 'Observers' and 'Notifications' into the global scope
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 10
Assigned To: Dão Gottwald [:dao]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-29 10:18 PDT by Dão Gottwald [:dao]
Modified: 2011-10-03 19:22 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.14 KB, patch)
2011-09-29 10:18 PDT, Dão Gottwald [:dao]
gavin.sharp: review+
Details | Diff | Review

Description Dão Gottwald [:dao] 2011-09-29 10:18:16 PDT
Created attachment 563449 [details] [diff] [review]
patch
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-09-29 10:26:17 PDT
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] 2011-09-29 10:56:25 PDT
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: gavin@gavinsharp.com] 2011-09-29 11:12:53 PDT
I don't think adding 1 more reference to it will significantly hinder efforts to rename it.
Comment 4 Dão Gottwald [:dao] 2011-09-29 11:39:15 PDT
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] 2011-10-03 07:57:26 PDT
https://hg.mozilla.org/mozilla-central/rev/e4ee17a3f362
Comment 6 Philipp von Weitershausen [:philikon] 2011-10-03 19:22:20 PDT
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).

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