Closed Bug 753765 Opened 9 years ago Closed 8 years ago

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

Categories

(SeaMonkey :: UI Design, defect)

defect
Not set
major

Tracking

(seamonkey2.9 wontfix, seamonkey2.10 wontfix, seamonkey2.11 verified, seamonkey2.12 verified)

VERIFIED FIXED
seamonkey2.13
Tracking Status
seamonkey2.9 --- wontfix
seamonkey2.10 --- wontfix
seamonkey2.11 --- verified
seamonkey2.12 --- verified

People

(Reporter: sgautherie, Assigned: neil)

References

(Depends on 1 open bug, )

Details

Attachments

(2 files)

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...
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".)
Severity: normal → major
Attachment #625175 - Flags: review?(iann_bugzilla) → review+
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
Attachment #625175 - Attachment description: (Av1) Remove an obsolete comment about bug 303327 → (Av1) Remove an obsolete comment about bug 303327 [Checked in: Comment 3]
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();
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #633614 - Flags: review?(iann_bugzilla)
Attachment #633614 - Flags: feedback?(sgautherie.bz)
Attachment #633614 - Flags: review?(iann_bugzilla) → review+
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.
Attachment #633614 - Flags: feedback?(sgautherie.bz)
Attachment #633614 - Flags: feedback+
Attachment #633614 - Flags: approval-comm-beta?
Attachment #633614 - Flags: approval-comm-aurora?
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.
Attachment #625175 - Flags: approval-comm-beta?
Pushed comm-central changeset 4d1465a48f5a.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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.
(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.
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
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Target Milestone: --- → seamonkey2.13
Attachment #625175 - Flags: approval-comm-beta? → approval-comm-beta+
Attachment #633614 - Flags: approval-comm-beta?
Attachment #633614 - Flags: approval-comm-beta+
Attachment #633614 - Flags: approval-comm-aurora?
Attachment #633614 - Flags: approval-comm-aurora+
Keywords: checkin-needed
Whiteboard: [c-n: 4d1465a48f5a to c-a, 9c72014bef78 and 4d1465a48f5a to c-b]
Attachment #633614 - Attachment description: Proposed patch → Proposed patch [Checked in: Comment 8]
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.)
Whiteboard: [c-n: 4d1465a48f5a to c-a, 9c72014bef78 and 4d1465a48f5a to c-b] → [c-n: 9c72014bef78 to c-b]
Keywords: checkin-needed
Keywords: checkin-needed
Attachment #633614 - Attachment description: Proposed patch [Checked in: Comment 8] → Proposed patch [Checked in: Comment 8 & 12]
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.
Attachment #625175 - Attachment description: (Av1) Remove an obsolete comment about bug 303327 [Checked in: Comment 3] → (Av1) Remove an obsolete comment about bug 303327 [Checked in: Comment 3 & 14]
You need to log in before you can comment on or make changes to this bug.