1278130 - Port Bug 1273685 PopupBlocking:UpdateBlockedPopups messages can be very large to Seamonkey

Status: RESOLVED FIXED

(SeaMonkey :: Passwords & Permissions, defect)

Description

[Frank-Rainer Grahl (:frg)]

Showing individual popup windows is currently broken in the status and notification bar. 

Instead you see an error in the web console.

>> Timestamp: 6/5/2016 10:15:31 PM
>> Error: TypeError: popups[i] is undefined
>> Source File: chrome://communicator/content/utilityOverlay.js
>> Line: 1287

Bug 1273685 needs to be ported.

Comment 1

[Frank-Rainer Grahl (:frg)]

Attachment: 1278130-popupblocking.patch

A patch! A patch!

Comment 2

[Frank-Rainer Grahl (:frg)]

Attachment: 1278130-popupblocking-V1a.patch

oops...

Comment 3

[Philip Chee]

Comment on attachment 8763336 [details] [diff] [review]
1278130-popupblocking-V1a.patch

> +                (oldBrowser.blockedPopups && !newBrowser.blockedPopups) ||
> +                (!oldBrowser.blockedPopups && newBrowser.blockedPopups))
This looks like it's attempting to execute a logical XOR. If so you could do the following:
  (!oldBrowser.blockedPopups != !newBrowser.blockedPopups)

> +      // Check for duplicates and reuse the old menuitem.
I think this is wrong. Every time you call retrieveListOfBlockedPopups() you get a potentially different list of popups. We should call RemovePopupsItems() every time we call onpopuphiding.

Should we port this bit too?
https://hg.mozilla.org/mozilla-central/annotate/01748a2b1a463f24efd9cd8abad9ccfd76b037b8/browser/base/content/browser.js#l557

Otherwise this looks reasonable.

Comment 4

[Frank-Rainer Grahl (:frg)]

Attachment: 1278130-popupblocking-V2.patch

[Approval Request Comment]
Regression caused by (bug #): 1273685
User impact if declined: individual popup blocking does not work
Testing completed (on m-c, etc.): c-a c-b
Risk to taking this patch (and alternatives if risky): non already broken
String changes made by this patch: none.


xor comment addressed. review+ from Philip Chee carried forward.

>> I think this is wrong. Every time you call retrieveListOfBlockedPopups() you 
>> get a potentially different list of popups. We should call 
>> RemovePopupsItems() every time we call onpopuphiding.

Looking at the code I think RemovePopupsItems() is called already. This is imho only for cases where there are duplicate items in the list itself. I updated the comment for it. Lmk if you think I am wrong here.

>> Should we port this bit too?

Yupp but I would opt for a separate bug.

Comment 5

[Frank-Rainer Grahl (:frg)]

>> >> Should we port this bit too?

>> Yupp but I would opt for a separate bug.

I now looked up what the code actually does. Does no longer apply after I reworked the menu in Bug 1055954. The status bar and preferences bar menus do not use a seperator.

Comment 6

[Philip Chee]

Comment on attachment 8785597 [details] [diff] [review]
1278130-popupblocking-V2.patch

(In reply to Frank-Rainer Grahl from comment #5)
> >> >> Should we port this bit too?
> 
> >> Yupp but I would opt for a separate bug.
> 
> I now looked up what the code actually does. Does no longer apply after I
> reworked the menu in Bug 1055954. The status bar and preferences bar menus
> do not use a seperator.
Sounds good. a=me for landing on comm-aurora and comm-beta

Comment 7

[Frank-Rainer Grahl (:frg)]

Locked and bolted:

https://hg.mozilla.org/comm-central/rev/ae490e13b85f
https://hg.mozilla.org/releases/comm-aurora/rev/30533da241d1
https://hg.mozilla.org/releases/comm-beta/rev/6ba8abbc0969