350353 - notificationbox is racy

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Michael Wu [:mwu]]

If notifications are added to the notificationbox too quickly, the notification box gets confused.

Comment 1

[Michael Wu [:mwu]]

Attachment: A symptom of a race in the notificationbox

Comment 2

[Michael Wu [:mwu]]

Attachment: Move important stuff out of animation callback

This patch makes notificationbox not depend on the completion of the animation.

Comment 3

[Michael Wu [:mwu]]

Attachment: Move important stuff out of animation callback, v2

Make sure the notification is visible if we're killing its animation callback to put a new notification in.

Comment 4

[Michael Wu [:mwu]]

Attachment: Move important stuff out of animation callback, v3

Last patch broke the animated closing. This version doesn't.

Comment 5

[Neil Deakin]

Comment on attachment 236013 [details] [diff] [review]
Move important stuff out of animation callback, v3

This appears to be a branch patch, which doesn't have 343574 in it.

I tested the patch but it didn't seem to work properly when closing a notification when two were open, testing with http://xulplanet.com/ndeakin/tests/xts/notification/notificationbox-browser.xul

Comment 6

[Michael Wu [:mwu]]

Attachment: Move important stuff out of animation callback, v4

Ported to trunk version of notification.xml, tested/debugged with http://xulplanet.com/ndeakin/tests/xts/notification/notificationbox-browser.xul

Comment 7

[Michael Wu [:mwu]]

Checking in toolkit/content/widgets/notification.xml;
/cvsroot/mozilla/toolkit/content/widgets/notification.xml,v  <--  notification.xml
new revision: 1.8; previous revision: 1.7
done

Comment 8

[Michael Wu [:mwu]]

Comment on attachment 236980 [details] [diff] [review]
Move important stuff out of animation callback, v4

This patch is necessary if we want bug 348576. It fixes regressions due to the animation code that is used now and makes it possible to open two notifications at the same time without racing.

Comment 9

[Michael Wu [:mwu]]

Attachment: Move important stuff out of animation callback, v4 (branch)

Comment 10

[Mike Schroepfer]

Comment on attachment 237775 [details] [diff] [review]
Move important stuff out of animation callback, v4 (branch)

a=schrep for regression fix in new functionality.

Comment 11

[Michael Wu [:mwu]]

Branch:
Checking in toolkit/content/widgets/notification.xml;
/cvsroot/mozilla/toolkit/content/widgets/notification.xml,v  <--  notification.xml
new revision: 1.1.2.7; previous revision: 1.1.2.6
done

Comment 12

[(not reading, please use [e-mail] instead)]

robert, is this the bug you were showing me today?

Comment 13

[Robert Strong (they/them - no direct email)]

Hey Sir Seth,
This is for the race condition that mwu found.
bug 348576 is for getting the add-ons mgr. to use notificationbox.
I don't believe there is a bug for the notificationbox displaying full height then animating from a min height back to full height but I haven't searched for one as of yet. I have a couple of ideas of how this can be improved / fixed for 3.0