Last Comment Bug 753765 - SeaMonkey browser_popupNotification.js: investigate "the logic taken from bug 575957 fails to work here, work around it for now."
: SeaMonkey browser_popupNotification.js: investigate "the logic taken from bug...
Status: VERIFIED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: All All
: -- major (vote)
: seamonkey2.13
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
http://mxr.mozilla.org/comm-central/f...
Depends on: 575957 sm-doorhanger
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-10 07:01 PDT by Serge Gautherie (:sgautherie)
Modified: 2012-07-09 07:34 PDT (History)
3 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wontfix
wontfix
verified
verified


Attachments
(Av1) Remove an obsolete comment about bug 303327 [Checked in: Comment 3 & 14] (947 bytes, patch)
2012-05-18 11:18 PDT, Serge Gautherie (:sgautherie)
iann_bugzilla: review+
iann_bugzilla: approval‑comm‑beta+
Details | Diff | Splinter Review
Proposed patch [Checked in: Comment 8 & 12] (2.20 KB, patch)
2012-06-15 11:55 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
bugzillamozillaorg_serge_20140323: feedback+
iann_bugzilla: approval‑comm‑aurora+
iann_bugzilla: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Serge Gautherie (:sgautherie) 2012-05-10 07:01:29 PDT
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) 2012-05-18 11:11:21 PDT
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) 2012-05-18 11:18:16 PDT
Created attachment 625175 [details] [diff] [review]
(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) 2012-05-19 16:06:51 PDT
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 neil@parkwaycc.co.uk 2012-06-15 10:10:22 PDT
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 neil@parkwaycc.co.uk 2012-06-15 11:55:22 PDT
Created attachment 633614 [details] [diff] [review]
Proposed patch
[Checked in: Comment 8 & 12]
Comment 6 Serge Gautherie (:sgautherie) 2012-06-27 23:46:29 PDT
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, sgautherie.bz@free.fr-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) 2012-06-27 23:48:49 PDT
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 neil@parkwaycc.co.uk 2012-06-28 12:12:19 PDT
Pushed comm-central changeset 4d1465a48f5a.
Comment 9 Justin Wood (:Callek) 2012-07-04 21:54:18 PDT
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 neil@parkwaycc.co.uk 2012-07-05 00:29:01 PDT
(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) 2012-07-05 03:18:01 PDT
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 neil@parkwaycc.co.uk 2012-07-06 12:01:03 PDT
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) 2012-07-07 04:28:28 PDT
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] 2012-07-07 06:19:10 PDT
https://hg.mozilla.org/releases/comm-beta/rev/84722fe83bd3

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