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

VERIFIED FIXED in seamonkey2.13

Status

SeaMonkey
UI Design
--
major
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: sgautherie, Assigned: neil@parkwaycc.co.uk)

Tracking

(Depends on: 1 bug)

Trunk
seamonkey2.13
Dependency tree / graph
Bug Flags:
in-testsuite +

SeaMonkey Tracking Flags

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

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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...
(Reporter)

Comment 1

5 years ago
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
status-seamonkey2.10: --- → affected
status-seamonkey2.11: --- → affected
status-seamonkey2.9: --- → wontfix
tracking-seamonkey2.10: --- → ?
(Reporter)

Comment 2

5 years ago
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?).
Attachment #625175 - Flags: review?(iann_bugzilla)

Updated

5 years ago
Attachment #625175 - Flags: review?(iann_bugzilla) → review+
(Reporter)

Comment 3

5 years ago
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]
(Assignee)

Comment 4

5 years ago
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)

Comment 5

5 years ago
Created attachment 633614 [details] [diff] [review]
Proposed patch
[Checked in: Comment 8 & 12]
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #633614 - Flags: review?(iann_bugzilla)
Attachment #633614 - Flags: feedback?(sgautherie.bz)
(Reporter)

Updated

5 years ago
status-seamonkey2.10: affected → wontfix
status-seamonkey2.12: --- → affected
tracking-seamonkey2.10: ? → ---
tracking-seamonkey2.11: --- → ?

Updated

5 years ago
Attachment #633614 - Flags: review?(iann_bugzilla) → review+
(Reporter)

Comment 6

5 years ago
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?
(Reporter)

Comment 7

5 years ago
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?
(Assignee)

Comment 8

5 years ago
Pushed comm-central changeset 4d1465a48f5a.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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.
(Assignee)

Comment 10

5 years ago
(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.
(Reporter)

Comment 11

5 years ago
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

Updated

5 years ago
Attachment #625175 - Flags: approval-comm-beta? → approval-comm-beta+

Updated

5 years ago
Attachment #633614 - Flags: approval-comm-beta?
Attachment #633614 - Flags: approval-comm-beta+
Attachment #633614 - Flags: approval-comm-aurora?
Attachment #633614 - Flags: approval-comm-aurora+
(Reporter)

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [c-n: 4d1465a48f5a to c-a, 9c72014bef78 and 4d1465a48f5a to c-b]
(Reporter)

Updated

5 years ago
Attachment #633614 - Attachment description: Proposed patch → Proposed patch [Checked in: Comment 8]
(Assignee)

Comment 12

5 years ago
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.)
status-seamonkey2.11: affected → fixed
status-seamonkey2.12: affected → fixed
Whiteboard: [c-n: 4d1465a48f5a to c-a, 9c72014bef78 and 4d1465a48f5a to c-b] → [c-n: 9c72014bef78 to c-b]

Updated

5 years ago
Keywords: checkin-needed
(Reporter)

Updated

5 years ago
Keywords: checkin-needed
(Reporter)

Updated

5 years ago
Attachment #633614 - Attachment description: Proposed patch [Checked in: Comment 8] → Proposed patch [Checked in: Comment 8 & 12]
(Reporter)

Comment 13

5 years ago
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.
status-seamonkey2.11: fixed → verified
status-seamonkey2.12: fixed → verified
tracking-seamonkey2.11: ? → ---
https://hg.mozilla.org/releases/comm-beta/rev/84722fe83bd3
Keywords: checkin-needed
Whiteboard: [c-n: 9c72014bef78 to c-b]
(Reporter)

Updated

5 years ago
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.