Closed Bug 353918 Opened 13 years ago Closed 13 years ago

No updates available notification does not go away when switching the view

Categories

(Toolkit :: Add-ons Manager, defect)

1.8 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.8.1

People

(Reporter: mwu, Assigned: mwu)

References

Details

(Keywords: regression, verified1.8.1.1)

Attachments

(1 file, 1 obsolete file)

After checking for new updates and not finding any, the no updates notification does not disappear if the user switches away from the extensions view to, for example, the themes view.
Attachment #239760 - Flags: review?(robert.bugzilla)
Comment on attachment 239760 [details] [diff] [review]
Fix noUpdatesDismiss once and for all

That looks like the correct fix to me but this r=me is also contingent on your testing that this does fix the problem.
Attachment #239760 - Flags: review?(robert.bugzilla) → review+
*sigh* My instincts told me to check clicking "find updates" twice, and I was right. I've updated this patch to provide more robustness against the race described in bug 352450. Basically, the anti-duplicate notification code tries to remove an old notification of the same type before adding it again, and is inadvertently being triggered when the old notification was already removed because it takes a moment before the old notification is really removed from the list. Now the code checks to see if the notification is really still there. The  code in noUpdatesDismiss that touched notification box was moved to the end of the function to minimize any problems if the function failed. I hate races.
Attachment #239760 - Attachment is obsolete: true
Attachment #239763 - Flags: review?(robert.bugzilla)
Flags: blocking-firefox2?
Target Milestone: --- → Firefox 2
Version: Trunk → unspecified
Status: NEW → ASSIGNED
Version: unspecified → 2.0 Branch
Comment on attachment 239763 [details] [diff] [review]
Fix noUpdatesDismiss once and for all, v2

This looks fine... I'm going to test it out before +'ing it.
I tested it on trunk and it appears to work fine for me.
Comment on attachment 239763 [details] [diff] [review]
Fix noUpdatesDismiss once and for all, v2

Thanks and it works fine for me. If you are comfortable with the patch as it stands please request a1.8.1
Attachment #239763 - Flags: review?(robert.bugzilla) → review+
Checking in toolkit/mozapps/extensions/content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <--  extensions.js
new revision: 1.117; previous revision: 1.116
done
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment on attachment 239763 [details] [diff] [review]
Fix noUpdatesDismiss once and for all, v2

I'm feel pretty good about this patch, now that I fully understand the nature of noUpdatesDismiss. This should be safe.
Attachment #239763 - Flags: approval1.8.1?
Comment on attachment 239763 [details] [diff] [review]
Fix noUpdatesDismiss once and for all, v2

Doesn't meet criteria for RC2.   i.e. "Show Stopper Bugs" - major regression, crash, security issue.  We can pickup in a follow-on release.
Attachment #239763 - Flags: approval1.8.1? → approval1.8.1-
(In reply to comment #9)
> (From update of attachment 239763 [details] [diff] [review] [edit])
> Doesn't meet criteria for RC2.   i.e. "Show Stopper Bugs" - major regression,
> crash, security issue.  We can pickup in a follow-on release.
> 

Well, this will cause major user confusion because it results in a situation where the window simultaneously lists updates to be installed with a big yellow banner across the top saying "No updates are available".

However, if you want to ship with this, I guess it's your call.
(In reply to comment #10)
> Well, this will cause major user confusion because it results in a situation
> where the window simultaneously lists updates to be installed with a big yellow
> banner across the top saying "No updates are available".
> 
Not likely under most circumstances. The user would have to update, not get any updates, leave the window open until an update is available, and then update again. The extensions updates notification will take care of updates for most people anyway. This bug can wait. (IMHO)
You are incorrect. It is extremely likely.  The user would merely have to check for extension updates, not find any, then switch to themes and find one.  This is exactly what happened to me that prompted me to reopen my original bug on this (bug 347568) that got pointed to this one.
(In reply to comment #12)
> You are incorrect. It is extremely likely.  The user would merely have to check
> for extension updates, not find any, then switch to themes and find one.  This
> is exactly what happened to me that prompted me to reopen my original bug on
> this (bug 347568) that got pointed to this one.
> 
Ah, that case is much more likely, but not exactly a show stopper either. Sorry I didn't find this regression in time. :/
Bill,

Thanks for your time an energy on this bug.  Shipping a release is always a a tough tradeoff.  If we hadn't found some severe crash issues in RC1 there would not have been an RC2 and this bug would not have been severe enough to cause an RC2 on its own.   The only way to safely ship is to stop making changes so we have a known entity.  We should take these changes on trunk and they can wind up a later security and stability release.   

Flags: blocking-firefox2? → blocking-firefox2-
Flags: blocking1.8.1.1?
Attachment #239763 - Flags: approval1.8.1.1?
Not a blocker but we'll look at the approval request
Flags: blocking1.8.1.1? → blocking1.8.1.1-
Comment on attachment 239763 [details] [diff] [review]
Fix noUpdatesDismiss once and for all, v2

approved for 1.8 branch, a=dveditz for drivers
Attachment #239763 - Flags: approval1.8.1.1? → approval1.8.1.1+
Checked in on branch.
Keywords: fixed1.8.1.1
verified using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.1pre) Gecko/20061130 BonEcho/2.0.0.1pre. I verified using the steps in the initial report, and it looks good. Adding keyword.
changing status to Verified per comment 18.
Status: RESOLVED → VERIFIED
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.