Closed Bug 120201 Opened 23 years ago Closed 22 years ago

Implement update notifications

Categories

(SeaMonkey :: UI Design, defect, P1)

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.9

People

(Reporter: samir_bugzilla, Assigned: samir_bugzilla)

References

()

Details

Attachments

(1 file, 3 obsolete files)

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
Blocks: 102472
QA Contact: sairuh → claudius
Note that this is not yet part of the installed build.  

law, please r.
dveditz, please sr.

Thanks.
Keywords: patch, review
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.
Depends on: 121506
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.





Attachment #68970 - Attachment is obsolete: true
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.
Attachment #69410 - Attachment is obsolete: true
Attachment #69410 - Flags: needs-work+
> 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.
Keywords: review
Whiteboard: [ready to checkin]
Checked in.  
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 ]
-------------------------------------------------
  
Marking fixed.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
I've files http://bugzilla.mozilla.org/show_bug.cgi?id=126582 on the missing
version number
Keywords: nsbeta1+
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
v
Status: RESOLVED → VERIFIED
Product: Core → Mozilla Application Suite
Blocks: 283580
Blocks: 271745
No longer blocks: 271745
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: