Closed Bug 307358 Opened 19 years ago Closed 18 years ago

Missing notification after auto-check finds updates to Extensions

Categories

(Toolkit :: Add-ons Manager, defect)

Product:
Toolkit
Component:
Add-ons Manager
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.8.1beta1

People

(Reporter: stebs, Assigned: mwu)

References

Details

(Keywords: fixed1.8.1, regression, Whiteboard: [SWAG: 2d])

Attachments

(3 files, 7 obsolete files)

Outdated Extensions for testing compressed as Zip
171.48 KB, application/octet-stream
Details
dirtiness around OS X button after install
18.35 KB, image/png
Details
Notify user on startup of new add-ons updates, v8
26.88 KB, patch
robert.strong.bugs
: review+
robert.strong.bugs
: approval-branch-1.8.1+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050906 Firefox/1.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050906 Firefox/1.4

If "Automatically check for updates to:" is enabled for "Deer Parc" (Firefox)
and "Installed Extensions and Themes", you get a notification if updates to Deer
Parc are found, but you DONT get notified for any update found for an Extension. 
The only way to see if there is an Extension-update available is to manually
open the EM, scroll through all the Extensions and look out for the "Update Now"
Button behind each Extension.
Since Extension-updates will never be applied automatically and most
unexperienced Users will not look regularly into EM, auto-check gets pretty
useless and most Users will be "stuck" with old Extensions.
This could be an potential security-risk with old unsafe Extensions (aka
Greasemonkey) not beeing updated. Therefore setting Severity to Major (and a
major feature is broken afterall). Please notice me if this should be lowered to
normal. 


Reproducible: Always

Steps to Reproduce:
1. Make sure you have "Automatically check for updates to:" enabled for Extensions
2. Install some old Extensions for which an Update is available and the
update-check works. (I took MR_Tech_About-About_1.1.1; linkification_0.10.0 and
flashgot-0.5.9.5) You may have to raise em:maxVersion to 1.4 in install.rdf with
latest Nightlies.
3. Set extensions.update.interval to something as low as 180 if you dont want to
wait 24h.
4. Restart Browser, browse normally and watch for any notification. I had to
wait approx. 10 minutes, propbably because of app.update.timer 600000. 

Actual Results:  
There were no visible notification, sign, etc. of any (Extension)-Update found,
no Update was downloaded or installed. The only way to see if the auto-check was
sucessful was to watch for the "Update Now" Buttons in EM.  

Expected Results:  
Some sort of notification that an Update to Extension X was found and asking if
it should be installed (just like Application-Updates), maybe even respect the
"Automatically donwload and install the update" setting...
Component: Software Update → Extension/Theme Manager
QA Contact: software.update → extension.manager
This are the outdated Extensions (with em:maxVersion raised to 1.4) I used for
testing, compressed as a zip-file.
This could easily be caused by bug 307235 and a patch for that bug has just been
checked in.
Unfortunately this was not fixed by Bug 307325. Tested with Mozilla/5.0
(Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4
ID:2005090806

Some easier way to verify this Bug:
Install at least one outdated Extensions, DONT hit "Find Updates" in Extension
Manager (EM). Check EM after 1-2 Days. If you see the "Update Now" Button behind
the outdated Extension and have not been informed with an Popup Notification
etc., you can confirm this Bug.  
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050908
Firefox/1.4 ID:2005090806


Please ignore my comment... I thought this was for the dialog not showing when
more than one update is found in the extension / theme manager which does work.

I don't consider updating extensions to be a good solution for extension
security in that updating can be turned off or there may not be an update
available that fixes the seurity issue. I do think this makes the auto checking
for updates to extensions for the most part useless if you have more than a
handful of extensions (e.g. if there are extensions out of view) or you have
several to update in which case it is easier to click "Find Updates" in order to
get the dialog so they can all be installed in one fell swoop.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.8b5?
Keywords: regression
Note: this was removed by the patch from bug 296566
somehow automatic extension update doesn't seem to work at all.
No notification and if i look in extensions ... after having the browser on for
almost 24 h there is no change there either.
extensions.update.interval is 86400 and
extensions.update.enabled is true
(In reply to comment #7)
> No notification
Yes, thats what I experienced in all my tests.

> and if i look in extensions ... after having the browser on for
> almost 24 h there is no change there either.
Almost 24 h is probably not enough...
Normally it has to be a bit over 24 h, and I got the feeling that it is
somethimes well over 24 h...
you could try with extensions.update.interval set to 180. You still have to wait
over 10 minutes...and make sure you do a restart after changing that value, I
think this is indeed necessary. 

After giving it enough time, auto-check was always working here, but pretty
useless since you have to lookout for those "Update Now" Buttons sometimes
hidden in the non-visible Part of the list.

It may sound extreme, but it would probably be better for Fx 1.5 to temporarily
hide (make unclickable etc.) the "Installed Extensions and Themes" check under
"Automatically check for updates to:".
Because: Right now the User is fooled to believe that Fx takes care of updating
its Extensions and Themes (didn't check what happens with Themes btw) by
updating them automatically or at least notify him upon an available update...
(And thus it is even less probably he checks his EM -which is an absolutely
necessary step atm)

 
I just shortened the timer and it found updates as it did for Stebs... I am very
sure that I have also seen extensions with updates available when opening the EM
so I don't think the underlying code is broken. The lack of user notification
without the user having to open the EM and examine the list is a bummer.
Rob, Mike, is this intended behavior then? I've found it quite surprising to
open up my extension manager and see several extensions with available updates.
I don't think people open the EM with any regularity so this seems less than ideal.
Flags: blocking1.8b5? → blocking1.8b5+
As I understood things, a dialog was to appear informing users when updates to
their extensions were available, but then that dialog would throw them over to
the EM for actual UI to apply those updates.

As Rob notes, bug 296566 seems to have removed the extension-related UI from the
update service, which is why (afaict) the dialog isn't there any more. Whether
or not that was an intended consequence is a question for Rob/Ben, I guess.

I don't see the problem with keeping the pop-up notifier in Software Update, and
having it throw over to EM if it detects extension updates, but I'm not aware of
the internal details there ...
It is intended in the sense that bug 296566 removed this but the original
reasons for having it are still valid as I see it. The previous notification was
the icon next to the throbber that would only be displayed when there were
updates. That had its own issues with not disappearing after updates were
applied but it might be possible to add something like that back.
I don't think we want to mix notification metaphors for updates if it can at all
be avoided. I think the best way to do this would be:

  - update service checks for app. update
  - if app. update found...
      :. and if auto-install is not checked, pop Software Update dialog 
      :. otherwise auto-intsall and then pop Software Updated! dialog
  - if app. update not found, checks for ext updates
  - if ext. update found, pops Software Update dialog (Extension Updates Found!)
      :. links to EM for extension update UI

This way there's a consistent means for notification of an update being found by
an automatic check, and a consistent location for turning on/off that notification.
That does sound better. I haven't spent much time with the software update code
or ui and adding this method would best be handled by Ben or Darin since they have.
cc'ing ben and darin
Ben - can you take a look?
QA Contact: extension.manager → beng.bugs
Assignee: nobody → beng.bugs
Consolidating under one account.
Assignee: beng.bugs → bugs
We can also make the EM sort updated items to the top (perhaps). 

You might consider this a regression technically but given the cryptic nature of
the christmas tree, and how few people ever likely interacted with it, the state
is much the same, if not slightly better as now when people go to the extension
manager they can see exactly which extensions have updates waiting. 

Yes we could implement a dialog of some sort here, but not without additional
documentation burden. Is this really a high priority for the 1.5 release?
marking blocking1.8b5? to cause triage team to revisit this
Flags: blocking1.8b5+ → blocking1.8b5?
Flags: blocking1.8b5? → blocking1.8b5-
(In reply to comment #18)
> Yes we could implement a dialog of some sort here, but not without additional
> documentation burden. Is this really a high priority for the 1.5 release?

Seeing as how none of the other dialogs are documented yet, I'm not exactly sure
how this becomes a documentation burden. Unless you mean a l10n burden, at which
point I sort of understand.

This is also a behavioural enhancement which can be delivered in a point
release, and I'd strongly encourage it to be considered for such.
*** Bug 311413 has been marked as a duplicate of this bug. ***
Flags: blocking1.8rc1?
Extension auto update just ran and this appeared in JavaScript Console, in
association:

Error: syntax error
Source File: http://extensions.meatme.net/update.rdf
Line: 1, Column: 63
Source Code:
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01
Transitional//EN">--------------------------------------------------------------^

That url to the update rdf is no longer valid and the js console message is just
informing you that it is not valid rdf... it has nothing to do with this bug.
per comments from ben and Mike Beltzner.

It's too late in the game to be figuring this out and it's not a stop ship
critical issue.
Flags: blocking1.8rc1? → blocking1.8rc1-
I'm not sure if this is the right bug for this because the other ones I've 
found all say something general like "extension autoupdate doesn't work." 
There are two problems I see that play a role in this unwanted behavior: 1) 
there is no UI notification, and 2) the timer code is faulty.

We had a "fun" time trying to get the app update timers working properly for 
1.5... see bug 309784. It seems like we're looking at the same kind of problem 
with the extension update timer as well. The basic problem is that firefox has 
to stay *running* for more than 24 hours before it will check for updates 
rather than looking after 24 hours have elapsed since the last check.

Does the timer stuff need it's own bug or should it get lumped into this one?

And, btw, I agree with Stebs: if we know for sure that extension autoupdate 
isn't going to work for 1.5 then I would vote to have the pref UI for that 
feature hidden.
OS: Windows XP → All
Hardware: PC → All
Ben: This should be working now.  At least it's working for me.

If you delete both app.update.lastUpdateTime.addon-background-update-timer and
app.update.lastUpdateTime.background-update-timer, the get recreated the next
time Firefox starts up.  I have also observed that their values also do advance
after 24+ hours.

The only possibility I see (speaking losely, as I have not reviewed the code),
if there in fact is a problem, is the potential that something similar to your
comment 18 ( https://bugzilla.mozilla.org/show_bug.cgi?id=309784#c18 ) for the
app might be affecting extension updates.
(In reply to comment #26)
> If you delete both app.update.lastUpdateTime.addon-background-update-timer and
> app.update.lastUpdateTime.background-update-timer, the get recreated the next
> time Firefox starts up.  I have also observed that their values also do 
advance
> after 24+ hours.

That's fine, but the extension update code is never checking the value of the 
addon-background-update-timer pref. The fact that it advances has to do with 
changes made in bug 309784, but this does not mean that the extension timer is 
firing when it should. Right now all that is happening is that a 24 hour timer 
is being installed each time the app runs. If the app is closed before those 24 
hours are up then the timer will be set to 24 hours again once the app is 
restarted. So, for example, if you restart firefox every 23 hours you will 
*never* have an autoupdate for extensions.

> The only possibility I see (speaking losely, as I have not reviewed the code),
> if there in fact is a problem, is the potential that something similar to your
> comment 18 ( https://bugzilla.mozilla.org/show_bug.cgi?id=309784#c18 ) for the
> app might be affecting extension updates.

This isn't an issue in the extension update code. That was just a typo that 
existed in the app update code.

so can this be fixed before Firefox 1.5 final?

because this is a very high-visibility bug, even for average users.

(In reply to comment #27)
 
> That's fine, but the extension update code is never checking the value of the 
> addon-background-update-timer pref. The fact that it advances has to do with 
> changes made in bug 309784, but this does not mean that the extension timer is 
> firing when it should. Right now all that is happening is that a 24 hour timer 
> is being installed each time the app runs. If the app is closed before those 24 
> hours are up then the timer will be set to 24 hours again once the app is 
> restarted. So, for example, if you restart firefox every 23 hours you will 
> *never* have an autoupdate for extensions.
> 

That is not happening to me.  My
app.update.lastUpdateTime.addon-background-update-timer does not roll over till
after 24 hours, and Firefox is automatically finding extension updates - but
just not providing a notification outside of Extension Manager.

Of course, like I said, I am ignorant with respect to the code.  In Firefox
1.0.x, there is extention update checking and notification code that works.  I
am assuming 1.5 reuses that code; or did someone leave that code in the app and
slap on additional extension checking code, such that what is actually working
for me is really the old code sans (i.e. broken) notification?  Just some thoughts.

In relation to the update timer for extension updates - it has been working for
me without the app running for 24 hours. Also, if the timer is failing I think
it should be a new bug.
Wow, sorry folks. Looks like the timer problem was resolved with the fix for 
bug 309784. The TimerManager is taking care of everything like it's supposed 
to. What's happening is that TimerManager is only calling the EM timer-
notification function after app.update.interval has elapsed AND 
extension.update.interval has elapsed since app.update.lastUpdateTime.addon-
background-update-timer. A little confusing, but it works, so scratch my timer 
complaint!
Since the extension update notification will be broken in Firefox 1.5, should the option to automatically check for Extension and theme be finaly removed? (idealy for rc2)
(In reply to comment #32)
> Since the extension update notification will be broken in Firefox 1.5, should
> the option to automatically check for Extension and theme be finaly removed?
> (idealy for rc2)
The only thing that is missing is the notification when updates are found. I think it is best to leave it as it is today for 1.5 so an extension can provide the missing notification... it may also provide ideas on how best to notify users in the browser ui when there are extension / theme updates.
> The only thing that is missing is the notification when updates are found. I
> think it is best to leave it as it is today for 1.5 so an extension can provide
> the missing notification... it may also provide ideas on how best to notify
> users in the browser ui when there are extension / theme updates.

I see your point, but rigth now your extension simply won't get updated even if the option is selected to update them automatically (there will be a update button on each extensions having updates pending in the extension manager but no automatic install)

There is not need to nuke the option alltogether, just hide it and perhaps put it to disable by default, this way it will still be possible to enable it with a extension or with about:config.

(In reply to comment #34)
> I see your point, but rigth now your extension simply won't get updated even if
> the option is selected to update them automatically (there will be a update
> button on each extensions having updates pending in the extension manager but
> no automatic install)
I don't know where automatic install comes into this... automatic install has nothing to do with this bug, was never implemented, and there were no plans for it to be implemented.

> There is not need to nuke the option alltogether, just hide it and perhaps put
> it to disable by default, this way it will still be possible to enable it with
> a extension or with about:config.
Being able to see there are updates available when opening the EM is a good thing and being able to enable / disable automatic checking for updates is also a good thing. Removing this from the ui is not the direction I think this should go though it would also be a good thing to make it clear that automatically checking for updates is not the same as automatic installation of updates or notification as is the case in this bug... I believe it is too late for that now except as part of the release notes. As I see it the value of removing it isn't great enough to warrant removing the ui at this stage.
> > I see your point, but rigth now your extension simply won't get updated even if
> > the option is selected to update them automatically (there will be a update
> > button on each extensions having updates pending in the extension manager but
> > no automatic install)
> I don't know where automatic install comes into this... automatic install has
> nothing to do with this bug, was never implemented, and there were no plans for
> it to be implemented.

I know that there no automatic install of extensions updates (and I hope there will never be one) but from the option description "Automatically check for updades to:" a user will imply that some notification will be given when updates are found or that the updates will simply get automatically applyed (like for anti-virus, anti-spyware, ...). But neither of those scenario will append in Firefox 1.5.

I do like the way it's work right now, it save me 1 mouse click and 5 secondes of waiting for extension update to download. But the name of the option is just deceptive when put in the same category as Firefox automatic check for Update.
Flags: blocking1.8.1?
Since it is now very clear that changing something about this bug for Firefox 1.5 is out of the question, I requested blocking for the next Product out.  Was a little confused though if blocking 1.8.1 or aviary2 was the right thing to do.
Is there a blocking flag for a future 1.5.0.x Release - or will those still be limited strictly to security/stability bugs?... If yes, a 1.5.1.x Release blocker?
Please change this if I picked the wrong one, thanks.
should be blocking aviary2 because it's not a specific gecko bug.
Flags: blocking-aviary2?
Flags: blocking1.8.1?
*** Bug 322721 has been marked as a duplicate of this bug. ***
This bit us hard in some user feedback (i.e. the tabprefs bustage, which was fixed, but users weren't finding the update).

-> Rob
Assignee: bugs → robert.bugzilla
Flags: blocking-firefox2? → blocking-firefox2+
QA Contact: beng.bugs → extension.manager
After discussing this with mconnor we decided to go with a notification dialog on startup prior to displaying the app ui. To accomplish this we would need a flag to check - probably a pref - to know we need to load the extensions datasource during the startup so we don't load it during every startup.
(In reply to comment #41)
> After discussing this with mconnor we decided to go with a notification dialog
> on startup prior to displaying the app ui.

I'm hoping that this will have a "Don't tell me again about this update" option in it. There are a couple of valid reasons for wanting to continue to use an older version and having this appear on every startup would be annoying. Ideally you could still hear about updates to other extensions, but not one's you've already seen.

I've been thinking on this a bit and with this occurring on startup along with what you mention has me considering using a notification similar to the app notification that won't make it into Firefox 2.0. More thought will be needed before any work commences on this.
(In reply to comment #43)
> I've been thinking on this a bit and with this occurring on startup along with
> what you mention has me considering using a notification similar to the app
> notification that won't make it into Firefox 2.0. More thought will be needed
> before any work commences on this.
> 

You could save the number of updated extensions and then only notify the user if this number changes.  The option to notify on updates to extensions and themes should probably be changed to contain 3 values (never, new updates only, any updates) instead of the current on/off value.
(In reply to comment #43)
> I've been thinking on this a bit and with this occurring on startup along with
> what you mention has me considering using a notification similar to the app
> notification that won't make it into Firefox 2.0. More thought will be needed
> before any work commences on this.
> 

In the mean time, it would perhaps be better to back out the patch from bug 296566 to get it working in Firefox 2.0.
(In reply to comment #44)
The number of available updates is an inaccurate way to track whether a new item has an update (e.g. it wouldn't cover the case of one item is updated and another item has a new available update). I've a couple of ideas how this can be better accomplished and I will investigate those when time permits which should be within the month.

(In reply to comment #45)
We are not going to throw the baby out with the bath water by backing out the patch from bug 296566. As I stated "ore thought will be needed before any work commences on this" and that is what is happening (e.g. please be patient).

(In reply to comment #46)

> We are not going to throw the baby out with the bath water by backing out the
> patch from bug 296566.

I was not meaning backing out the whole patch, but only the parts implying this bug.
It was decided a long time ago that the old "xmas tree" indicator had to go due to many people not knowing what it indicated along with other bugs with it like false indications. So, instead of working on creating a patch to only back out that portion and bringing that back I am going to work on figuring out a different solution.
(In reply to comment #48)
> It was decided a long time ago that the old "xmas tree" indicator had to go due
> to many people not knowing what it indicated along with other bugs with it like
> false indications. So, instead of working on creating a patch to only back out
> that portion and bringing that back I am going to work on figuring out a
> different solution.
> 

Is this referring to the green/red/blue indicator? I was just going to ask what happened to this as I remembered seeing it at one point. Then I saw this post. It would be great if "that portion" could be brought back or some facsimile thereof.
A new extension just came out that provides update notification in the Firefox 1.0.x style:
  https://addons.mozilla.org/extensions/moreinfo.php?id=2098&vid=12600

(In reply to comment #41)
> After discussing this with mconnor we decided to go with a
> notification dialog on startup prior to displaying the app ui.

Maybe this would be good for some people, but I would consider this a
complete non-fix.  I only restart Firefox when needed to work around
some bug (e.g., memory leak, some feature has stopped working, newly
installed extension doesn't work until restart).  Otherwise my Firefox
will run for months.

Like the Update Notifier extension, it would be fine to have an option (why not set by default) to automatically install extensions updates.
(In reply to comment #53)
> Like the Update Notifier extension, it would be fine to have an option (why not
> set by default) to automatically install extensions updates.
That is bug 321789
Please, don't install extension updates automatically by default.  Some extension updates have quality issues and cause crashes.  It is better that the user be forced to take action by default.  That way, when the browser is b0r3d, the user at least has some basis for determining "what happened?"

(In reply to comment #55)
Please read bug 321789 and unless you are going to fix this bug or have a comment that will help fix this bug please don't comment here.
Also see related problem in bug 316991.
*** Bug 329297 has been marked as a duplicate of this bug. ***
(In reply to comment #46)
> The number of available updates is an inaccurate way to track whether a new
> item has an update (e.g. it wouldn't cover the case of one item is updated and
> another item has a new available update). I've a couple of ideas how this can
> be better accomplished and I will investigate those when time permits which
> should be within the month.

The underlying rule should simply be: if an update to an add-on is available and the user has not previously been notified about that update, then the user should be notified. This supports the use case where a user dismisses and chooses to ignore the notification, and ensures that they don't miss any subsequent notifications.

The notification could be something as simple as a browsermessage that says "There are updates available for some of your add-ons  [Go to add-ons manager] [X]" (if we go this route, then we should depend on bug 268590). Or the dialog that you and mconnor were talking about.
(In reply to comment #59)
> The underlying rule should simply be: if an update to an add-on is available
> and the user has not previously been notified about that update, then the user
> should be notified. This supports the use case where a user dismisses and
> chooses to ignore the notification, and ensures that they don't miss any
> subsequent notifications.
Right... but this would require keeping tracking on a per addon basis a couple of additional data points.
* Is the update we just found the same as the previous update?
* Has the user been notified of the previous update?
* more? (I don't think so but perhaps)

I'd like to keep this simpler if at all possible and I believe it is possible... I dislike the idea of keeping track of 2 additional data points in prefs per addon but if we do end up going the route you describe that is what we'll end up doing.

something like
extensions.%EXT_ID%.updateAvailable.version
extensions.%EXT_ID%.updateAvailable.notified

> The notification could be something as simple as a browsermessage that says
> "There are updates available for some of your add-ons  [Go to add-ons manager]
> [X]" (if we go this route, then we should depend on bug 268590). Or the dialog
> that you and mconnor were talking about.
If you are willing to go with a browsermessage I'm game... browsermessage has been for notification of things related to the current page in the past so I didn't consider using it for this beyond wanting similar functionality for app notification. Also, I would much rather have this triggered by the update check than on startup.
(In reply to comment #60)
> Right... but this would require keeping tracking on a per addon basis a couple
> of additional data points.

Why? Why not just track the list of addons for which an update notification has been recieved? Let's say a user has addons A, B and C installed. They start the browser, and updates are available for A and B. We notify the user, and store:

updatesPendingFor={A,B}

Next time the browser starts, there are updates available for A and B. We compare that set to updatesPendingFor and notice there's no difference, so we do not notify the user. The user decides to update A. So now we modify what we're storing such that:

updatesPendingFor={B}

Next time the browser starts, there's an update for A again! So we compare and notice that updatesPendingFor does not contain A, and we notify the user. Next time the browser starts, there's updates for A, B and C, so we notify the user again.

> * Is the update we just found the same as the previous update?

Note that it's possible that B was updated more than once on AMO in that scenario. The user doesn't need to know that - they have been informed that they're using an out of date version of B, and their decision to not update it should be taken to mean that they are happy with their current version, no need to tell them when it goes from B' to B''.

> If you are willing to go with a browsermessage I'm game... browsermessage has
> been for notification of things related to the current page in the past so I

Mm, good point, but I don't know what other sort of notification we have available to us other than (ick, ugh) a modal dialog or (ick, arg) something like the christmas tree.

> notification. Also, I would much rather have this triggered by the update 
> check than on startup.

Oh, hrm, that makes browsermessage harder to do, since it'll pop up during their browsing experience, which means that they might associate it with some other action. Obviously I need to think the notification part of this through a little more. :(
Where is the data stored for update pending that is found on startup? If you take that into account and the second data point you mention of "updatesPendingFor={A,B}" that is two data points per extension. Also, prefs don't do arrays unless you do a delimited list which would be ugly, evil, <insert your favorite metaphor here> if there were more than a couple of extensions and we don't load the extensions datasource on startup for perf reasons unless there are changes to the installed extensions.

I've got a couple of ideas to handle this but I prefer having the user explicitly setting an extension to not be checked for updates vs. assuming that they aren't interested in updates due to not applying an update. For example, they may choose not to apply an update due to being busy when they receive the notification and under the scenario you propose they would never be notified again for the addons they received notifications for. They also may not be interested in an update this month but want an update 6 months from now for what ever reason (e.g. the moon happens to be blue) :P
BTW: When I say I would prefer that it was triggered by the notification check I do mean that literally and am reconsidering notification on startup. The problem with on startup is that we have to have at minimum a setting that triggers this on startup and people that don't restart - admittedly an edgecase - won't be notified. This really shouldn't be dependent on the application starting up and there are quirks depending on when this happens during startup as can be seen with some of the strange bugs for the compatibility check wizard during an upgrade. I would also like to use the Extension Manager ui for this instead of creating new ui or refurbishing the compatibility check wizard to support this as it did in the past. The wins with doing this at startup are we can restart to complete the install of the updates without needing to restore the user session and we can use dialogs since there is no session to interrupt at this time.
Considering that extension update only take effect after a restart, would it be "good enough" to do the check at shutdown rather than startup.  Users are already accustom to unsaved-changes or closing-multiple-tabs dialogues at shutdown.  Maybe a updated-extension dialogue at shutdown would be better received and more likely accepted since the update can take place without interrupting the users work flow.
(In reply to comment #64)
> Considering that extension update only take effect after a restart, would it be
> "good enough" to do the check at shutdown rather than startup.  Users are
> already accustom to unsaved-changes or closing-multiple-tabs dialogues at
> shutdown.  Maybe a updated-extension dialogue at shutdown would be better
> received and more likely accepted since the update can take place without
> interrupting the users work flow.
> 
And make it shut down even slower ?
Read Bug 328598 and think again ;-) 

(In reply to comment #65)
> And make it shut down even slower ?
> Read Bug 328598 and think again ;-) 
>
Even if it was implemented this way, there is no way to be sure the user saw the update notification on shutdown since the O/S might shut down the browser if the user just logs out, etc.


(In reply to comment #62)
> Where is the data stored for update pending that is found on startup? 

That's programming magic. I only speak pseudocode ;)

> they may choose not to apply an update due to being busy when they receive the
> notification and under the scenario you propose they would never be notified
> again for the addons they received notifications for. They also may not be

Yeah, that's a good point. I feel like the real question we're drilling down to here is: what are the use cases surrounding *not* automatically installing extension updates? Put another way, why do we consider them different from app updates. It seems to me that the complexity is coming from the very fact that we're treating them differently, when I'd expect that most users think of an extension as part of the application (since they chose to install it).
It can check if the update is a new update based on whether the version of the update has changed since Ben added this to the extensions datasource a short while back and we have to load the extensions datasource when checking for updates anyways. It would then set a pref that would then trigger the display of the ui on next startup and load the extensions datasource. All available updates would be displayed and individual updates can be de-selected via a checkbox in order to not install that update. If it is desired to not check for updates to a specific addon we can add that to the EM ui (this is something I have already been thinking on).

As to why they are treated differently there are several reasons... for starters, we don't necessarily manage those updates or have significant quality control of the updates in general. We can discuss this more later if you'd like (e.g. I don't think this is a good place for a discussion of the pros and cons of this).
(In reply to comment #65)
> And make it shut down even slower ?

Okay, the check's execution doesn't have to take place at shutdown but the dialogue presentation could.  A few extra seconds for shutdown after a user has accepted an update prompt seem preferable to interrupting the user at startup when he obviously had some other task on his mind.

It doesn't even have to replace in-session notification.  It could be just a helpful reminder before the user runs off to watch Desperate Housewives (it's on) or whatever.
It takes more than a few seconds to download updates especially over dial-up (keep in mind there can be more than one update and that themes are usually somewhere between .5 MB to 1.5 MB in size) and if it is an OS shutdown that is causing the app to exit the update won't occur.
It could download the update in the background during the browsing session.
(In reply to comment #65)
> (In reply to comment #64)
> > Considering that extension update only take effect after a restart, would it be
> > "good enough" to do the check at shutdown rather than startup.  Users are
> > already accustom to unsaved-changes or closing-multiple-tabs dialogues at
> > shutdown.  Maybe a updated-extension dialogue at shutdown would be better
> > received and more likely accepted since the update can take place without
> > interrupting the users work flow.
> > 
> And make it shut down even slower ?
> Read Bug 328598 and think again ;-) 
> 

I'd rather it shut down slower than start up slower. You can walk away or do other things after shutting it down. When you want to bring it up, you want it NOW!


(In reply to comment #71)
> It could download the update in the background during the browsing session.
This would make a fairly complex piece of code a very complex piece of code. Also, it still doesn't handle an OS shutdown as mentioned at least twice earlier or if the background download hasn't completed.

As for startup being slower... you can always cancel the update and update by opening the EM when ever you do have time. Also, the case where a person is shutting down because they have to leave for what ever reason is just as if not more compelling a reason not to do this on shutdown.
> Also, it still doesn't handle an OS shutdown as mentioned at least twice
> earlier or if the background download hasn't completed.

This OS shutdown argument is a red-herring.  I've had to cancel OS shutdowns when a closing programs reminded me of unsaved data.  I've also allowed unwanted data to be loss in other situations.  If the user wants, he can stop the OS shutdown to allow the upgrade, but more likely he will just allow the OS to do what it wants and upgrade later.  As for incomplete downloads, well then just don't attempt an upgrade.  Why bother the user with what *can't* be done.
 
> As for startup being slower... you can always cancel the update and update
> by opening the EM when ever you do have time.

When I have time?  Do you mean, like when I'm *done* doing what I wanted to do?  This is exactly the type of interruption I want to avoid when I start a task. If I'm to be interrupted, I rather it be when I'm done than when I'm about to start.

> Also, the case where a person is shutting down because they have to leave
> for what ever reason is just as if not more compelling a reason not to do
> this on shutdown.

If I'm in a rush and a closing program pops up some warning, I do what I have to do.  In this case I could click "cancel" or "later" or whatever and walk away or, more likely, just click "okay" to the upgrade and *still* walk away!  It's not like I have to wait around for the browser to reboot, unlike the case during startup.  When I shutdown the computer or even just close a program to move on to another, I don't sit around and babysit it till it's done.  I just deal with any unsaved data warnings, or in this case an upgrade-available warning, then go take care of business while the computer takes care of its.

Again, this doesn't have to replace in session notification. In fact, you could bring back the much maligned christmas tree or whatever and draw attention to it in the on shutdown upgrade-warning using large images and detailed text - even offer a "never bug me again at shutdown" option.


(In reply to comment #74)
> > Also, it still doesn't handle an OS shutdown as mentioned at least twice
> > earlier or if the background download hasn't completed.
> 
> This OS shutdown argument is a red-herring.  I've had to cancel OS shutdowns
> when a closing programs reminded me of unsaved data.  I've also allowed
> unwanted data to be loss in other situations.  If the user wants, he can stop
> the OS shutdown to allow the upgrade, but more likely he will just allow the OS
> to do what it wants and upgrade later.  As for incomplete downloads, well then
> just don't attempt an upgrade.  Why bother the user with what *can't* be done.
So, more code to handle this case... n thank you. Also, just because you have had to cancel OS shutdowns with some apps doesn't mean an app should enter a state where it causes the user to be notified that it is preventing a shutdown if it can be helped which it can in this case simply by not doing this on shutdown.

> > As for startup being slower... you can always cancel the update and update
> > by opening the EM when ever you do have time.
> 
> When I have time?  Do you mean, like when I'm *done* doing what I wanted to do?
>  This is exactly the type of interruption I want to avoid when I start a task.
> If I'm to be interrupted, I rather it be when I'm done than when I'm about to
> start.
If you are too busy to install updates on start you can simply not install updates on start. The same arguments you bring up can be made for doing it on shutdown.

> > Also, the case where a person is shutting down because they have to leave
> > for what ever reason is just as if not more compelling a reason not to do
> > this on shutdown.
> 
> If I'm in a rush and a closing program pops up some warning, I do what I have
> to do.  In this case I could click "cancel" or "later" or whatever and walk
> away or, more likely, just click "okay" to the upgrade and *still* walk away! 
> It's not like I have to wait around for the browser to reboot, unlike the case
> during startup.  When I shutdown the computer or even just close a program to
> move on to another, I don't sit around and babysit it till it's done.  I just
> deal with any unsaved data warnings, or in this case an upgrade-available
> warning, then go take care of business while the computer takes care of its.
And you could get a cup of coffee while the updates are installed on startup.

> Again, this doesn't have to replace in session notification. In fact, you could
> bring back the much maligned christmas tree or whatever and draw attention to
> it in the on shutdown upgrade-warning using large images and detailed text -
> even offer a "never bug me again at shutdown" option.
There is no in session notification and the christmas tree was removed by the person that implemented it due to complaints and bugs... I personally have no problem with it but I am not going to reimplement it due to those complaints and bugs.

There is no solution to this problem that will please everyone. I am willing to implement the on startup solution but I would prefer a notification when the update check is made during a session as I have stated several times before (e.g. I am not a big fan of on startup notification - I just realize it is the lesser of the evils with what we have to work with). To implement a notification in session we need an application notification solution similar to the browsermessage ui except for the application obviously. This new ui didn't make the cut for 2.0... perhaps you or someone else has the time to implement it so this could then utilize it?
Robert,

This isn't an attack on you, your work, or your planned implementation.  It is simply a discussion of possibilities.  That said, I found your responses to be wholly unconvincing.  However, maybe this discussion should move to some other forum so you can get on to doing what you have to do.

Thanks for all the hard work, both past and future.  My exploration into the code is only beginning.  I can only hope that one day I'll be able to contribute even a shadow of what you have.
No worries and I am aware it isn't an attack on me. :P

My concerns regarding doing this on shutdown haven't been addressed except to state "I've had to cancel OS shutdowns when a closing programs reminded me of unsaved data". That doesn't mean we should do this if it can be avoided. It also adds complexity to the code which wouldn't be needed otherwise.

The arguments for shutdown have been in regards to being interrupted on startup. The same arguments apply to being interrupted on shutdown... it comes down to a subjective preference at this point with some people preferring to perform this task at the start of using an app vs. performing this task at the end of using an app. The one main difference between the two is that we can avoid the additional complexity needed to properly handle this on shutdown when doing this on startup. So far there hasn't been anything that convinces me that doing this on shutdown is preferable for a concrete reason.

More discussion on this subject should happen in the forums and I started a thread... if there are compelling reasons that you can provide I am very interested in reading them there.
http://forums.mozillazine.org/viewtopic.php?t=393058
Target Milestone: --- → Firefox 2 alpha2
Target Milestone: Firefox 2 alpha2 → Firefox 2 beta1
I thought this bug was about popping up a notifying window to warn the user that add-ons updates are available (with a link the the add-on manager included in the pop-up.

I read the last comments and it seems to be about when to automatically download and install the updates. That should be a separate bug IMO. Firefox already checks for updates on add-ons, this bug should be about getting a pop-up, or icon or something so the user is able to easily know that they are available.
Assignee: robert.bugzilla → nobody
Assignee: nobody → michael.wu
Whiteboard: 2d → [SWAG: 2d]
This patch:

1. Changes the update timer to 1 week instead of 1 day for checking for add-ons updates.
2. Maintains a list of updates to extensions that have not yet been installed. This allows the code to determine if there is a new update that the user probably has not seen yet, thus should set a boolean pref to true to show the notification on next startup.
3. Sets that pref to false when the notification is shown *or* the user opens up the add-ons manager. Actually, the notification is just a special mode added to the add-ons manager that is triggered by passing the argument "updates-only" while opening the add-ons manager. This dialog both notifies the user of new updates and allows installation of the new updates.
Attachment #225103 - Flags: review?(robert.bugzilla)
Comment on attachment 225103 [details] [diff] [review]
Notify user on startup of new add-ons updates

Removing review. I talked with Michael about this needing to restart so that new components, chrome, etc. will be available afterwards.
Attachment #225103 - Flags: review?(robert.bugzilla)
This version adds icons to the close button, properly disables the close button until updates are done installing, restarts firefox if updates are installed, and changes the add-ons update interval to 2 days.
Attachment #225103 - Attachment is obsolete: true
Attachment #225201 - Flags: review?(robert.bugzilla)
The changes in showView were modified to integrate with the existing code a bit better. An unnecessary test in the changes to nsExtensionManager.js.in was removed. Otherwise, it's just more of the same, with cleaner code.
Attachment #225201 - Attachment is obsolete: true
Attachment #225202 - Flags: review?(robert.bugzilla)
Attachment #225201 - Flags: review?(robert.bugzilla)
Attachment #225202 - Flags: ui-review?(beltzner)
Comment on attachment 225202 [details] [diff] [review]
Notify user on startup of new add-ons updates, v3

Thanks Michael!

>--- toolkit/mozapps/extensions/content/extensions.js	5 Jun 2006 21:45:08 -0000	1.72.2.20
>+++ toolkit/mozapps/extensions/content/extensions.js	11 Jun 2006 19:49:19 -0000
...
>@@ -242,16 +245,18 @@ function showView(aView) {
...
>+      if (window.arguments == "updates-only")
>+        showSkip = true;
Use windows.arguments[0]
I suspect this is named updates-only so that it can support opening for updates during normal operation of the app? If so, good call.

>@@ -265,17 +270,20 @@ function showView(aView) {
>                       ["typeName", "update"] ];
>       types = [ [ ["availableUpdateVersion", "?availableUpdateVersion", null] ] ];
>       break;
>     case "installs":
>       document.getElementById("installs-view").hidden = false;
>       showInstallFile = false;
>       showCheckUpdatesAll = false;
>       showInstallUpdatesAll = false;
>-      showRestartApp = true;
>+      if (window.arguments == "updates-only")
Use windows.arguments[0]

>@@ -505,16 +515,26 @@ function Startup() 
...
>+      else if (window.arguments[0] == "updates-only") {
>+        document.getElementById("viewGroup").hidden = true;
>+        document.getElementById("extensionsView").setAttribute("norestart", "");
Is "norestart" the right name for this attribute? It is unclear to me how "norestart" is applicable here.

>@@ -1316,16 +1338,21 @@ function checkUpdatesAll() {
...
>+  if (window.arguments == "updates-only") {
>+    var addonsMsg = document.getElementById("addonsMsg");
>+    addonsMsg.hideMessage();
>+  }
>+
Use windows.arguments[0]

>--- toolkit/mozapps/extensions/content/extensions.xml	5 Jun 2006 21:48:13 -0000	1.17.2.3
>+++ toolkit/mozapps/extensions/content/extensions.xml	11 Jun 2006 19:49:19 -0000
>@@ -453,16 +453,22 @@
...
>+  <binding id="addon-install-updated">
>+    <content>
>+      <xul:label value="&updateSuccess.label;" flex="1" crop="end"/>
>+    </content>
>+  </binding>
Can / should this be used for updates when the mgr. is opened from within the app?


>--- toolkit/mozapps/extensions/src/nsExtensionManager.js.in	5 Jun 2006 21:45:08 -0000	1.144.2.44
>+++ toolkit/mozapps/extensions/src/nsExtensionManager.js.in	11 Jun 2006 19:49:27 -0000
...
>@@ -2775,17 +2778,43 @@ ExtensionManager.prototype = {
...
>+
>+  /**
>+   * Notify user that there are new addons updates
>+   */
>+  _addonsUpdateNotify: function() {
Notify tends to be for notification. How about _showUpdatesWindow... this follows this file's convention for displaying a new window.

>@@ -2894,18 +2923,75 @@ ExtensionManager.prototype = {
...
>+    var self = this;
>+    function AddonsBackgroundObserver() {}
>+    AddonsBackgroundObserver.prototype = {
>+      _showUpdates: false,
>+      _updatedExtensions: null,
>+      _oldUpdateList: null,
Instead of setting all extensions and their version with an update as a string in a pref how about using availableUpdateVersion in the extensions datasource? Seems like it would be cleaner this way and the property is already available.

>--- browser/app/profile/firefox.js	9 May 2006 00:23:10 -0000	1.71.2.37
>+++ browser/app/profile/firefox.js	11 Jun 2006 19:16:57 -0000
>@@ -140,18 +140,18 @@ pref("app.update.incompatible.mode", 0);
...
>-pref("extensions.update.interval", 86400);  // Check for updates to Extensions and 
>-                                            // Themes every week
>+pref("extensions.update.interval", 172800);  // Check for updates to Extensions and 
>+                                             // Themes every two days
We're just going to go with checking every day. Fixing or removing the comment would be a good thing.
Attachment #225202 - Flags: review?(robert.bugzilla) → review-
Comment on attachment 225202 [details] [diff] [review]
Notify user on startup of new add-ons updates, v3

clearing review request, will wait for next patch :)
Attachment #225202 - Flags: ui-review?(beltzner)
(In reply to comment #83)
> >--- toolkit/mozapps/extensions/content/extensions.js	5 Jun 2006 21:45:08 -0000	1.72.2.20
> >+++ toolkit/mozapps/extensions/content/extensions.js	11 Jun 2006 19:49:19 -0000
> ...
> >@@ -242,16 +245,18 @@ function showView(aView) {
> ...
> >+      if (window.arguments == "updates-only")
> >+        showSkip = true;
> Use windows.arguments[0]
> I suspect this is named updates-only so that it can support opening for updates
> during normal operation of the app? If so, good call.
> 
No, actually, I just thought it was a good name. :) I just needed to tell whether or not the addons manager should switch to a special startup updates mode.

> >@@ -505,16 +515,26 @@ function Startup() 
> ...
> >+      else if (window.arguments[0] == "updates-only") {
> >+        document.getElementById("viewGroup").hidden = true;
> >+        document.getElementById("extensionsView").setAttribute("norestart", "");
> Is "norestart" the right name for this attribute? It is unclear to me how
> "norestart" is applicable here.
> 
It is used by a css selector to change the message on the extension after a successful update from "Restart to complete the installation." to "Update completed successfully."

> >--- toolkit/mozapps/extensions/content/extensions.xml	5 Jun 2006 21:48:13 -0000	1.17.2.3
> >+++ toolkit/mozapps/extensions/content/extensions.xml	11 Jun 2006 19:49:19 -0000
> >@@ -453,16 +453,22 @@
> ...
> >+  <binding id="addon-install-updated">
> >+    <content>
> >+      <xul:label value="&updateSuccess.label;" flex="1" crop="end"/>
> >+    </content>
> >+  </binding>
> Can / should this be used for updates when the mgr. is opened from within the
> app?
> 
I don't think so, because within the app, the restart is very visible, unlike opening at startup.

> >@@ -2894,18 +2923,75 @@ ExtensionManager.prototype = {
> ...
> >+    var self = this;
> >+    function AddonsBackgroundObserver() {}
> >+    AddonsBackgroundObserver.prototype = {
> >+      _showUpdates: false,
> >+      _updatedExtensions: null,
> >+      _oldUpdateList: null,
> Instead of setting all extensions and their version with an update as a string
> in a pref how about using availableUpdateVersion in the extensions datasource?
> Seems like it would be cleaner this way and the property is already available.
> 
The problem is that if the user skips the update for some extension, we don't want to show the update again, because the user already knows about it. However, we also want to show the update dialog if a different extension has received an update when it didn't have any before. We also avoid popping up the updates dialog for a new update when there was already a notification for an older update, because the user should already know that extension needs to be updated. 

I have applied all other changes that I didn't comment on. Thanks!
Updated with rob's comments.
Attachment #225202 - Attachment is obsolete: true
Attachment #225891 - Flags: ui-review?(beltzner)
Attachment #225891 - Flags: review?(robert.bugzilla)
Use info from RDF to determine if we need to show the notification. Make app updater disable the extension updater on startup.
Attachment #225891 - Attachment is obsolete: true
Attachment #225949 - Flags: ui-review?(beltzner)
Attachment #225949 - Flags: review?(robert.bugzilla)
Attachment #225891 - Flags: ui-review?(beltzner)
Attachment #225891 - Flags: review?(robert.bugzilla)
Better version with some more of rob's suggestions.
Attachment #225949 - Attachment is obsolete: true
Attachment #225956 - Flags: ui-review?(beltzner)
Attachment #225956 - Flags: review?(robert.bugzilla)
Attachment #225949 - Flags: ui-review?(beltzner)
Attachment #225949 - Flags: review?(robert.bugzilla)
Comment on attachment 225956 [details] [diff] [review]
Notify user on startup of new add-ons updates, v6

>--- toolkit/mozapps/extensions/content/extensions.css	3 May 2006 01:19:50 -0000	1.9.2.2
>+++ toolkit/mozapps/extensions/content/extensions.css	17 Jun 2006 02:18:10 -0000
The same needs to be done for pinstripe.

>--- toolkit/mozapps/extensions/content/extensions.js	15 Jun 2006 21:04:19 -0000	1.72.2.24
>+++ toolkit/mozapps/extensions/content/extensions.js	17 Jun 2006 02:18:10 -0000
>@@ -505,16 +516,27 @@ function Startup() 
>   if ("arguments" in window) {
>     try {
>       var params = window.arguments[0].QueryInterface(Components.interfaces.nsIDialogParamBlock);
>       gDownloadManager.addDownloads(params);
>     }
>     catch (e) {
>       if (window.arguments[0] == "updatecheck")
>         checkUpdatesAll();
>+      else if (window.arguments[0] == "updates-only") {
>+        gUpdatesOnly = true;
>+        document.getElementById("viewGroup").hidden = true;
>+        document.getElementById("extensionsView").setAttribute("norestart", "");
>+        showView('updates');
>+        var addonsMsg = document.getElementById("addonsMsg");
>+        addonsMsg.showMessage("chrome://mozapps/skin/extensions/question.png",
>+                              getExtensionString("newUpdatesAvailableMsg"),
>+                              null, null, true, null);
>+        window.title = getExtensionString("newUpdateWindowTitle", [getBrandShortName()]);
>+      }
>     }
>   }
> 
>   if (gExtensionsView.selectedItem)
>     gExtensionsView.scrollBoxObject.scrollToElement(gExtensionsView.selectedItem);
> 
>   if (!gCheckCompat) {
>     var msgText = getExtensionString("disabledCompatMsg");
Move the "if (!gCheckCompat) {..." and "if (gInSafeMode) {..." blocks before the gDownloadManager = new XPInstallDownloadManager(); block. Otherwise the updates message will not display when those conditions are met.

Would it be better to remove _PREF_UPDATE_NOTIFYUSER from extensions.js and just reset it in nsExtensionManager.js.in _showUpdatesWindow after the try catch block?

>--- toolkit/mozapps/extensions/src/nsExtensionManager.js.in	15 Jun 2006 21:07:50 -0000	1.144.2.47
>+++ toolkit/mozapps/extensions/src/nsExtensionManager.js.in	17 Jun 2006 02:18:11 -0000
>@@ -72,16 +72,18 @@ const PREF_DSS_SKIN_TO_SELECT         = 
...
>+const PREF_UPDATE_LASTSET             = "extensions.update.lastSet";
This should be removed... correct?

>@@ -2895,18 +2924,66 @@ ExtensionManager.prototype = {
...
>+      onUpdateEnded: function() {
>+        gPref.setCharPref(PREF_UPDATE_LASTSET, this._updatedExtensions.join());
This should be removed... correct?

...
>+        if (this._showUpdates)
>+          gPref.setBoolPref(PREF_UPDATE_NOTIFYUSER, true);
I'm wondering if this shouldn't happen in onAddonUpdateEnded. While testing I shutdown while an update check was happening and it didn't get set. This is probably somewhat of an edgecase but I think it is worth fixing since the cost is low.

...
>+
>+    this.update([], 0, false, new AddonsBackgroundObserver());
Shouldn't this be
var ds = this.datasource;
this.update([], 0, false, new AddonsBackgroundObserver(ds));
Attachment #225956 - Flags: review?(robert.bugzilla) → review-
Comment on attachment 225956 [details] [diff] [review]
Notify user on startup of new add-ons updates, v6

>--- toolkit/mozapps/extensions/src/nsExtensionManager.js.in	15 Jun 2006 21:07:50 -0000	1.144.2.47
>+++ toolkit/mozapps/extensions/src/nsExtensionManager.js.in	17 Jun 2006 02:18:11 -0000
>@@ -72,16 +72,18 @@ const PREF_DSS_SKIN_TO_SELECT         = 

>+  _showUpdatesWindow: function() {
>+    try {
>+      if (!gPref.getBoolPref(PREF_UPDATE_NOTIFYUSER))
>+        return;
Use the getPref helper function instead.
(In reply to comment #89)
> (From update of attachment 225956 [details] [diff] [review] [edit])
> >--- toolkit/mozapps/extensions/content/extensions.css	3 May 2006 01:19:50 -0000	1.9.2.2
> >+++ toolkit/mozapps/extensions/content/extensions.css	17 Jun 2006 02:18:10 -0000
> The same needs to be done for pinstripe.
> 
Huh? This isn't in winstripe.. Also, the winstripe changes aren't carried over to pinstripe since the buttons on the bottom of the extension manager in osx do not have icons.

> >--- toolkit/mozapps/extensions/content/extensions.js	15 Jun 2006 21:04:19 -0000	1.72.2.24
> >+++ toolkit/mozapps/extensions/content/extensions.js	17 Jun 2006 02:18:10 -0000
> >@@ -505,16 +516,27 @@ function Startup() 
> >   if ("arguments" in window) {
> >     try {
> >       var params = window.arguments[0].QueryInterface(Components.interfaces.nsIDialogParamBlock);
> >       gDownloadManager.addDownloads(params);
> >     }
> >     catch (e) {
> >       if (window.arguments[0] == "updatecheck")
> >         checkUpdatesAll();
> >+      else if (window.arguments[0] == "updates-only") {
> >+        gUpdatesOnly = true;
> >+        document.getElementById("viewGroup").hidden = true;
> >+        document.getElementById("extensionsView").setAttribute("norestart", "");
> >+        showView('updates');
> >+        var addonsMsg = document.getElementById("addonsMsg");
> >+        addonsMsg.showMessage("chrome://mozapps/skin/extensions/question.png",
> >+                              getExtensionString("newUpdatesAvailableMsg"),
> >+                              null, null, true, null);
> >+        window.title = getExtensionString("newUpdateWindowTitle", [getBrandShortName()]);
> >+      }
> >     }
> >   }
> > 
> >   if (gExtensionsView.selectedItem)
> >     gExtensionsView.scrollBoxObject.scrollToElement(gExtensionsView.selectedItem);
> > 
> >   if (!gCheckCompat) {
> >     var msgText = getExtensionString("disabledCompatMsg");
> Move the "if (!gCheckCompat) {..." and "if (gInSafeMode) {..." blocks before
> the gDownloadManager = new XPInstallDownloadManager(); block. Otherwise the
> updates message will not display when those conditions are met.
> 
Good point.

> Would it be better to remove _PREF_UPDATE_NOTIFYUSER from extensions.js and
> just reset it in nsExtensionManager.js.in _showUpdatesWindow after the try
> catch block?
> 
No, because we don't want to show the update dialog when the user opens the extension manager manually after the background update, because the user should see the updates when he/she does that.

> >--- toolkit/mozapps/extensions/src/nsExtensionManager.js.in	15 Jun 2006 21:07:50 -0000	1.144.2.47
> >+++ toolkit/mozapps/extensions/src/nsExtensionManager.js.in	17 Jun 2006 02:18:11 -0000
> >@@ -72,16 +72,18 @@ const PREF_DSS_SKIN_TO_SELECT         = 
> ...
> >+const PREF_UPDATE_LASTSET             = "extensions.update.lastSet";
> This should be removed... correct?
> 
> >@@ -2895,18 +2924,66 @@ ExtensionManager.prototype = {
> ...
> >+      onUpdateEnded: function() {
> >+        gPref.setCharPref(PREF_UPDATE_LASTSET, this._updatedExtensions.join());
> This should be removed... correct?
> 
Opps. Done.

> ...
> >+        if (this._showUpdates)
> >+          gPref.setBoolPref(PREF_UPDATE_NOTIFYUSER, true);
> I'm wondering if this shouldn't happen in onAddonUpdateEnded. While testing I
> shutdown while an update check was happening and it didn't get set. This is
> probably somewhat of an edgecase but I think it is worth fixing since the cost
> is low.
> 
You're right, continuously setting PREF_UPDATE_NOTIFYUSER as new extensions get updates is the correct behavior, in case the user opens the extension manager in the midst of the background update. Though.. I think there may be a problem if that happens and then the user hits find updates. I'm gonna reset PREF_UPDATE_NOTIFYUSER when the user initiates a check for updates.

> ...
> >+
> >+    this.update([], 0, false, new AddonsBackgroundObserver());
> Shouldn't this be
> var ds = this.datasource;
> this.update([], 0, false, new AddonsBackgroundObserver(ds));
> 
Yup.


        Attachment #225956 -
        Attachment is obsolete: true

        Attachment #226092 -
        Flags: ui-review?(beltzner)

        Attachment #226092 -
        Flags: review?(robert.bugzilla)

        Attachment #225956 -
        Flags: ui-review?(beltzner)

    
    

    
  
(In reply to comment #91)
> Huh? This isn't in winstripe.. Also, the winstripe changes aren't carried over
> to pinstripe since the buttons on the bottom of the extension manager in osx do
> not have icons.
Sorry, I just commented on the first extensions.css after seeing that winstripe had a change without updating pinstripe... I meant to comment on this one.
--- toolkit/themes/winstripe/mozapps/extensions/extensions.css	30 May 2006 06:16:27 -0000	1.16.2.8
+++ toolkit/themes/winstripe/mozapps/extensions/extensions.css	17 Jun 2006 01:20:11 -0000

> > >--- toolkit/mozapps/extensions/content/extensions.js	15 Jun 2006 21:04:19 -0000	1.72.2.24
> > >+++ toolkit/mozapps/extensions/content/extensions.js	17 Jun 2006 02:18:10 -0000
... 
> > Would it be better to remove _PREF_UPDATE_NOTIFYUSER from extensions.js and
> > just reset it in nsExtensionManager.js.in _showUpdatesWindow after the try
> > catch block?
> > 
> No, because we don't want to show the update dialog when the user opens the
> extension manager manually after the background update, because the user should
> see the updates when he/she does that.
How could that happen since you use window.arpuments to show the update dialog and not the pref?

I spent a little time with this code today and thought it might be cleaner to just create an internal version of update named updateInternal that takes an additional argument of backgroundCheck and do everything there instead of in the listener. update could just call that function with false for backgroundCheck. See installItemFromFile and installItemFromFileInternal for an example.

    
    

    
  
(In reply to comment #93)
> > > >--- toolkit/mozapps/extensions/content/extensions.js	15 Jun 2006 21:04:19 -0000	1.72.2.24
> > > >+++ toolkit/mozapps/extensions/content/extensions.js	17 Jun 2006 02:18:10 -0000
> ... 
> > > Would it be better to remove _PREF_UPDATE_NOTIFYUSER from extensions.js and
> > > just reset it in nsExtensionManager.js.in _showUpdatesWindow after the try
> > > catch block?
> > > 
> > No, because we don't want to show the update dialog when the user opens the
> > extension manager manually after the background update, because the user should
> > see the updates when he/she does that.
> How could that happen since you use window.arpuments to show the update dialog
> and not the pref?
doh! I see what you are getting at. You clear the pref since the user should see that the Updates view is available. I'm fine with that.

    
    

    
  
Comment on attachment 226092 [details] [diff] [review]
Notify user on startup of new add-ons updates, v7

I tested this a little bit, and the primary interaction model and strings seem to work out. Some nits:

- if I deselect all of the available updates, "Install Updates" should become disabled

- the default action should be "Install Updates"

- I'm still a little unclear on what happens on "Skip" - do I get notified again on the next startup? On the next update?

        Attachment #226092 -
        Flags: ui-review?(beltzner) → ui-review+

    
    

    
  
Forgot that pinstripe doesn't use images on the buttons so the css changes aren't needed. (In reply to comment #91)

> (In reply to comment #89)
> You're right, continuously setting PREF_UPDATE_NOTIFYUSER as new extensions get
> updates is the correct behavior, in case the user opens the extension manager
> in the midst of the background update. Though.. I think there may be a problem
> if that happens and then the user hits find updates. I'm gonna reset
> PREF_UPDATE_NOTIFYUSER when the user initiates a check for updates.
I was referring to shutting down during a background check... then the pref won't get set.

    
    

    
  


  
Michael: on OS X I played around with clicking the "Install Updates" button when I had no updates selected, then I selected an update and clicked "Install Updates" and what seemed to happen was that some of the button highlight state remained while the button vanished. Here's a screenshot (see lower left corner).

    
    

    
  
(In reply to comment #95)
> (From update of attachment 226092 [details] [diff] [review] [edit])
> I tested this a little bit, and the primary interaction model and strings seem
> to work out. Some nits:
> 
> - if I deselect all of the available updates, "Install Updates" should become
> disabled
> 
Same for the regular addons manager. A separate bug should be filed for this.

> - the default action should be "Install Updates"
> 
Can do.

> - I'm still a little unclear on what happens on "Skip" - do I get notified
> again on the next startup? On the next update?
> 
The tooltip says "skip these updates", so you only get notified again when the background check finds a new update. (and you don't open the addons manager before the next restart to discover it)

Also, WRT that cruft left over after installing, that bug also occurs with the regular add-ons manager. Should file a new bug for that.

    
    

    
  
(In reply to comment #98)
> (In reply to comment #95)
> > (From update of attachment 226092 [details] [diff] [review] [edit] [edit])
...
> > - if I deselect all of the available updates, "Install Updates" should become
> > disabled
> > 
> Same for the regular addons manager. A separate bug should be filed for this.
I was thinking about just displaying the message bar when the Install Updates button is clicked and there are no updates selected. This way we don't have to enumerate everything in the list to see if one is selected everytime a checkbox in the list is de-selected and the message can explain what is going on vs. just disabling the button.

> Also, WRT that cruft left over after installing, that bug also occurs with the
> regular add-ons manager. Should file a new bug for that.
Good catch beltzner, I have never seen that on Linux or Win32 and don't have a Mac.


    
    

    
  
Filed Bug 341978 for the leftover button cruft on Mac OS X.

    
    

    
  


  

        Attachment #226092 -
        Attachment is obsolete: true

        Attachment #226220 -
        Flags: review?(robert.bugzilla)

        Attachment #226092 -
        Flags: review?(robert.bugzilla)

        Attachment #226220 -
        Flags: review?(robert.bugzilla) → review+

    
    

    
  
Thanks Michael!

Filed Bug 342098 for a somewhat minor followup issue.

    
    

    
  
Checked in to trunk
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED

        Attachment #226220 -
        Flags: approval-branch-1.8.1+

    
    

    
  
Checked in to MOZILLA_1_8_BRANCH for Firefox 2.0
Keywords: fixed1.8.1

    
    

    
  
(In reply to comment #98)
>> - if I deselect all of the available updates, "Install Updates" should become
>> disabled
>> 
> Same for the regular addons manager. A separate bug should be filed for this.

Was a follow-up filed on this or should I be filing that?

    
    

    
  
(In reply to comment #105)
> Was a follow-up filed on this or should I be filing that?
Filed Bug 342329


    
  
Blocks: 354007
Product: Firefox → Toolkit

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: