All users were logged out of Bugzilla on October 13th, 2018

Implement update notifications

VERIFIED FIXED in mozilla0.9.9

Status

P1
normal
VERIFIED FIXED
17 years ago
5 years ago

People

(Reporter: samir_bugzilla, Assigned: samir_bugzilla)

Tracking

Trunk
mozilla0.9.9
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

17 years ago
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.
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla0.9.9
(Assignee)

Updated

17 years ago
Blocks: 102472
QA Contact: sairuh → claudius
(Assignee)

Comment 1

17 years ago
Created attachment 66214 [details] [diff] [review]
nsUpdateNotifier JS component and update notification additions to the Software Installation pref panel.
(Assignee)

Comment 2

17 years ago
Note that this is not yet part of the installed build.  

law, please r.
dveditz, please sr.

Thanks.
Keywords: patch, review
(Assignee)

Comment 3

17 years ago
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>

Comment 4

17 years ago
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.
(Assignee)

Comment 6

17 years ago
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.

Comment 7

17 years ago
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.
(Assignee)

Updated

17 years ago
Depends on: 121506
(Assignee)

Comment 8

17 years ago
Created attachment 68970 [details] [diff] [review]
Patch to remove dependency on the hidden window and address other reviewer comments.

law, please r.
dveditz, please sr.

Thanks.
Attachment #66214 - Attachment is obsolete: true

Comment 9

17 years ago
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.





(Assignee)

Comment 10

17 years ago
Created attachment 69410 [details] [diff] [review]
Patch rev 3 addressing Bill's last round of comments.
Attachment #68970 - Attachment is obsolete: true
(Assignee)

Comment 11

17 years ago
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+
(Assignee)

Comment 13

17 years ago
> 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.

(Assignee)

Comment 14

17 years ago
Created attachment 70142 [details] [diff] [review]
Patch rev 4 addressing all items in comment 12.
(Assignee)

Comment 15

17 years ago
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+
(Assignee)

Comment 17

17 years ago
> 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 18

17 years ago
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+
(Assignee)

Comment 19

17 years ago
Pavlov's comments addressed.
Keywords: review
Whiteboard: [ready to checkin]
(Assignee)

Comment 20

17 years ago
Checked in.  
Whiteboard: [ready to checkin]

Comment 21

17 years ago
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 ]
-------------------------------------------------
  
(Assignee)

Comment 22

17 years ago
Marking fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 23

17 years ago
I've files http://bugzilla.mozilla.org/show_bug.cgi?id=126582 on the missing
version number

Updated

17 years ago
Keywords: nsbeta1+

Comment 24

17 years ago
is this supposed to work ? i got mine set to daily and i never got a message
when new versions came out.

Comment 25

17 years ago
> 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.

Comment 26

17 years ago
shouldnt it be removed from the preferences for 1.0 then ? it will only confuse
people

Comment 27

16 years ago
v
Status: RESOLVED → VERIFIED
Product: Core → Mozilla Application Suite

Updated

14 years ago
Blocks: 283580

Updated

14 years ago
Blocks: 271745

Updated

14 years ago
No longer blocks: 271745
You need to log in before you can comment on or make changes to this bug.