Closed Bug 120201 Opened 21 years ago Closed 21 years ago
Implement update notifications
We should poll a remote RDF datasource containing information about the latest client update 5 minutes after startup (to avoid affecting startup performance). If the remote datasource indicates there is a newer version of the software (comparing it to the version registry through XPInstall) then we alert the user as such. For more details see the spec pointed to by this bug's URL field.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla0.9.9
Note that this is not yet part of the installed build. law, please r. dveditz, please sr. Thanks.
Reviewers and developers interested in the design of the update notification component may find the following UML diagrams helpful: <http://www.mozilla.org/xpapps/updates/design/sequence-diagram.png> <http://www.mozilla.org/xpapps/updates/design/class-diagram.png>
Samir, the code looks good; two minor issues: 1. Can you convert the dump() call in registerSelf to a debug() call? 2. Do you need to add clobber:: rules to the makefiles so that the .js file to undo the install:: rules? I like there to be symmetry there, but maybe I'm too picky. I have one major issue: Is there some alternative to using the infamous "hidden window?" I spoke to danm about this and he would hate to see a new dependency on it. It looks like you're accessing two properties of that window: setTimeout and InstallTrigger. I wonder if setTimeout wouldn't work without the hidden window? I'm not sure exactly what kind of JS context components execute under but perhaps setTimeout works like dump() and just works? I don't know anything about InstallTrigger, though.
When InstallTrigger is made into a scriptable component Samir can get one from the Components object, but for now the hybrid thing in the DOM is all that's available.
Actually, |setTimeout()| hangs off of a DOM window (see nsIDOMWindowInternal). Initially, I assumed it was part of core JS but it isn't and so it didn't work. I could get the ``activeWindow'' from the window watcher service: the profile dialog is up by the time we need to call |setTimeout()|. Note, we are also using the hidden window for a third purpose: to open the vendor update page (|gWindow.open()| call). I don't suppose that the profile window going away will be a problem. We will need to get the ``activeWindow'' twice probably (or even three times) since the second and third times when we use it to call into |InstallTrigger.compareVersion()| and |open()|, respectively, the profile dialog won't be around. I don't think getting the ``activeWindow'' twice (or every time used) will cause a performance hit.
You can do the |open()| using the nsIWindowWatcher service, so that one's not a problem. Is using the "active window" always going to work? Sometimes there isn't an active window (but I'm not sure this code kicks in in that case). Maybe I need a little background info on what's going on here (in layman's terms). I know I'd like that background info. Anyway, I can buy off on using the hidden window, but only if danm says its OK. Another solution that works would be preferable, I think.
law, please r. dveditz, please sr. Thanks.
Attachment #66214 - Attachment is obsolete: true
r=law, with the following comments... >+ var kITimerScriptable = Components.interfaces.nsITimerScriptable; That could be a "const." >+ void init(in nsIObserver aObserver, in unsigned long aDelay, >+ in unsigned long aPriority, in unsigned long aType); I think there should be some comments describing the arguments (e.g., is aDelay in seconds, milliseconds, microseconds?). >+// For reference (from nsTimerImpl.h): >+// #define NS_TIMER_CONTRACTID "@mozilla.org/timer;1" Why not put a real #define in (enclosed with the appropriate C++ mumbo-jumbo)? Also, there should be a similar #define ("for reference" or not), for the aTopic passed to the observer.
Comment on attachment 69410 [details] [diff] [review] Patch rev 3 addressing Bill's last round of comments. r=law
Attachment #69410 - Flags: review+
Comment on attachment 69410 [details] [diff] [review] Patch rev 3 addressing Bill's last round of comments. >--- /dev/null Wed Feb 13 19:04:56 2002 >+++ xpfe/components/updates/src/nsUpdateNotifier.js Wed Feb 13 16:05:22 2002 What kind of diff program made this? If you cvs add the files then cvs diff -N will get them as part of a normal diff, and then reviewers worry less about tree bustage due to "oops, forgot to check in a file", the #1 most common reason. >+ var updateDatasourceURI = prefs.getCharPref(kUNDatasourceURIPref); This should probably be getLocalizedUnicharPref() >+++ xpfe/components/updates/src/makefile.win Sat Feb 9 17:10:48 2002 >+install:: nsUpdateNotifier.js >+ !@$(MAKE_INSTALL) $** $(DIST)\bin\components >+ >+clobber:: >+ rm -f $(DIST)\bin\components\nsUpdateNotifier.js We don't use the install:: phase anymore, it's libs::, and the clobber:: step should be clobber_all:: >+ /* Timer priorities */ >+ const short kPriorityHighest = 10; >+ const short kPriorityHigh = 8; >+ const short kPriorityNormal = 5; >+ const short kPriorityLow = 2; >+ const short kPriorityLowest = 0; >+ >+ /* Timer types */ >+ const short kTypeOneShot = 0; >+ const short kTypeRepeatingSlack = 1; >+ const short kTypeRepeatingPrecise = 2; UPPER_CASE is preferred for constants in IDL files. >+ /** >+ * Initialize a timer that will fire after the said delay. >+ * A user must keep a reference to this timer till it is >+ * is no longer needed or has been cancelled. >+ * >+ * @param aObserver the object to callback when the timer fires Shouldn't you specify what it'll observe? That is, you might pass in a class that's observing multiple things, so they'll want to know what topic to expect, what the subject will be, etc. >Index: xpcom/threads/Makefile.in >Index: xpcom/threads/makefile.win Where's the mac version to process/export this new .idl file? I want someone like pavlov, dougt or brendan to r= the timer changes. The look fine to me, but I don't want to be surprised or be a surprise to someone else's long-range plans for frozeness. >+<!ENTITY updateNotifications.desc "Check for new versions of this software. \ >+ No information about you or your computer will be revealed during this operation."> This is not strictly true, we'll get all the usual header stuff including IP address (information about your computer) and if HTTP is used cookies will automatically be sent (information about you). I think simpler is better than appearing to promise things we can't. >+pref("update_notifications.provider.0.datasource_uri", "http://update-notification-server.com/updates.rdf"); URL's are stored as the name of a property file, and GetLocalizedPref() is used to get their value. Is "_uri" on the end necessary? Seems like either "datasource" or "uri" would be enough, both is redundant.
> What kind of diff program made this? If you cvs add the files then > cvs diff -N will get them as part of a normal diff, and then reviewers > worry less about tree bustage due to "oops, forgot to check in a file", > the #1 most common reason. Didn't want to add the directries in case this was going into the commercial tree only. So I used ``diff -N -u /dev/null nsUpdateNotifier.js''. I have a script that generates this patch. I will be using it as a manifest when I do the checkin. > Where's the mac version to process/export this new .idl file? Oops, my script diff'ed MANIFEST instead of MANIFEST_IDL. Will be in next patch. > I think simpler is better than appearing to promise things we can't. The UI verbiage was straight from the spec. I'll delete this false promise in the upcoming patch revision. > Is "_uri" on the end necessary? Seems like either "datasource" or "uri" > would be enough, both is redundant. Stuck with ``datasource'' only. All other comments also addressed in the upcoming patch revision.
Pavlov, Please review the timer portion of patch rev 4 (attachment 70142 [details] [diff] [review]).
Comment on attachment 70142 [details] [diff] [review] Patch rev 4 addressing all items in comment 12. >Index: xpfe/communicator/resources/locale/en-US/region.properties >=================================================================== >+# update notifications: new update page >+update_notifications.provider.0.datasource=http://update-notifications-server.com/updates.rdf This should be somewhere on mozilla.org, right? For testing purposes you can always set a location in your profile. I'd rather have this blank or pointing to a dead link on mozilla.org than pointing off into space that might get hijacked. There's a commercial version of this file too, you'll need to add an appropriate http://home.netscape.com/bookmark/ url there (that can be a separate bug). You need to document the format of updates.rdf somewhere. A readme in your update notification directory would be sufficient, or even just an example updates.rdf. sr=dveditz on the conditions - pav or another timer owner gives r= on the timer code - you change the updates.rdf server to something safer - you file a bugscape bug to get a URL for commercial
Attachment #70142 - Flags: superreview+
> You need to document the format of updates.rdf somewhere. Added a README.html explaining the datasource pref and format to the src directory. > I'd rather have this blank... > - you change the updates.rdf server to something safer Done. Points at nothing. > - you file a bugscape bug to get a URL for commercial Bug <http://bugscape/show_bug.cgi?id=12193> filed.
Comment on attachment 70142 [details] [diff] [review] Patch rev 4 addressing all items in comment 12. Please add a comment in nsITimerScriptable.idl saying that the interface is temporary until nsITimer is converted to idl. I hope to get to this for 1.0. r=pavlov
Attachment #70142 - Flags: review+
Pavlov's comments addressed.
Whiteboard: [ready to checkin]
Whiteboard: [ready to checkin]
There should be added a version number to the "Mozilla Update Notification" dialog so that the dialog would look like: ---- Mozilla Update Notification ---------------- There is an update available for Mozilla. Your version: 0.9.9 Available version: 1.0.0 Would you like to get the update? [ Ok ] [ Cancel ] -------------------------------------------------
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
I've files http://bugzilla.mozilla.org/show_bug.cgi?id=126582 on the missing version number
is this supposed to work ? i got mine set to daily and i never got a message when new versions came out.
> is this supposed to work ? i got mine set to daily and i never got a message > when new versions came out. I don't think Mozilla makes use of this feature. Some distributors (e.g. Netscape) do though.
shouldnt it be removed from the preferences for 1.0 then ? it will only confuse people
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.