753765 - SeaMonkey browser_popupNotification.js: investigate "the logic taken from bug 575957 fails to work here, work around it for now."

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Serge Gautherie (:sgautherie)]

Bug 570004 added the following block to SeaMonkey test:
{
267     },
268     // XXX: the logic taken from bug 575957 fails to work here, work around it for now.
269     updateNotShowing: function () {
270       todo_is(PopupNotifications.isPanelOpen, true, "panel should be open");
271       gBrowser.selectedTab = this.oldSelectedTab;
272       wrongBrowserNotification.remove();
273       wrongBrowserNotification = null;
}

At some point, FF and SM (test) should be fixed/resync'ed...

Comment 1

[Serge Gautherie (:sgautherie)]

SM 2.12a1:
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1337348393.1337352489.23433.gz&fulltext=1
WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/05/18 06:39:53
{
TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | [Test #4] running test
TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | [Test #4] observer for updateNotShowing called
TEST-KNOWN-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | panel should be open
}

Added updateNotShowing() is called instead of onShown() and onHidden() :-|

***

Ftr,
{
237   // test opening a notification for a background browser
238   { // Test #3

252   // now select that browser and test to see that the notification appeared
253   { // Test #4
}

***

This looks like a real application issue:
see bug 575957 comment 2
and existing code in browser/base/content/browser.js

Iiuc, SeaMonkey would be calling removeTransientNotifications() in a case it should not.
(Unless the test code itself is "wrong".)

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Remove an obsolete comment about bug 303327 [Checked in: Comment 3 & 14]

Bug 303327 has been R.WFM.
And
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/notification.xml#200
has no parameter (anymore?).

Comment 3

[Serge Gautherie (:sgautherie)]

Comment on attachment 625175 [details] [diff] [review]
(Av1) Remove an obsolete comment about bug 303327
[Checked in: Comment 3 & 14]

SM 2.12a1:
http://hg.mozilla.org/comm-central/rev/9c72014bef78

Comment 4

[[e-mail]]

Hmm, my logic in nsBrowserStatusHandler might be wrong, I think it might get confused if you switch to a loading tab. Maybe this would be better:

if (aRequest && this.popupNotifications &&
    (aWebProgress.isLoadingDocument ||
     !Components.isSuccessCode(aRequest.status)))
  this.popupNotifications.locationChange();

Comment 5

[[e-mail]]

Attachment: Proposed patch [Checked in: Comment 8 & 12]

Comment 6

[Serge Gautherie (:sgautherie)]

Comment on attachment 633614 [details] [diff] [review]
Proposed patch
[Checked in: Comment 8 & 12]

[Mozilla/5.0 (Windows NT 5.1; rv:16.0) Gecko/16.0 Firefox/16.0 SeaMonkey/2.13a1 // Build identifier: 20120627041654] (MsVc8, [e-mail]-67dbc42bd72d/try-comm-central-win32)

This lets the test pass :-)


[Approval Request Comment]
User impact if declined: notifications (can) miss to display when loaded in background.
Risk to taking this patch (and alternatives if risky): low risk.

Comment 7

[Serge Gautherie (:sgautherie)]

Comment on attachment 625175 [details] [diff] [review]
(Av1) Remove an obsolete comment about bug 303327
[Checked in: Comment 3 & 14]

[Approval Request Comment]
User impact if declined: Only to ease version tracking in bugzilla.
Risk to taking this patch (and alternatives if risky): None, comment-only.

Comment 8

[[e-mail]]

Pushed comm-central changeset 4d1465a48f5a.

Comment 9

[Justin Wood (:Callek)]

Comment on attachment 633614 [details] [diff] [review]
Proposed patch
[Checked in: Comment 8 & 12]

IanN, or Neil, can you triage these approvals, I'm not sure on the risk vs reward assessment here.

Comment 10

[[e-mail]]

(In reply to Justin Wood from comment #9)
> I'm not sure on the risk vs reward assessment here.
The reward is that doorhangers in background tabs get shown properly.

Comment 11

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1341473203.1341476269.12844.gz&fulltext=1
WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/07/05 00:26:43
{
TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | [Test #4] added listeners; panel state: false
TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | [Test #4] running test
TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | [Test #4] popup showing
TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | [Test #4] popup shown
TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | [Test #4] checking popup
...
TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | [Test #4] popup hidden (0 hides remaining)
...
}

V.Fixed

Comment 12

[[e-mail]]

Pushed comm-aurora changeset 3d0360cd72e0.
Pushed comm-beta changeset 0eeeeefbecb0.

(Sorry, I pushed to beta first, before realising that attachment 625175 [details] [diff] [review] needed to go on beta too.)

Comment 13

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Aurora/1341614692.1341617628.31774.gz&fulltext=1
WINNT 5.2 comm-aurora debug test mochitest-other on 2012/07/06 15:44:52
+
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Beta/1341621253.1341625442.5669.gz&fulltext=1
WINNT 5.2 comm-beta debug test mochitest-other on 2012/07/06 17:34:13
{
TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | [Test #4] popup shown
...
TEST-INFO | chrome://mochitests/content/browser/suite/browser/test/browser_popupNotification.js | [Test #4] popup hidden (0 hides remaining)
}

seamonkey2.12 and seamonkey2.11: verified.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/comm-beta/rev/84722fe83bd3