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

RESOLVED FIXED

Status

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: frg, Assigned: frg)

Tracking

SeaMonkey Tracking Flags

(seamonkey2.46 fixed, seamonkey2.47 fixed, seamonkey2.48 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8763335 [details] [diff] [review]
1278130-popupblocking.patch

A patch! A patch!
Assignee: nobody → frgrahl
Status: NEW → ASSIGNED
Attachment #8763335 - Flags: review?(philip.chee)
(Assignee)

Updated

3 years ago
status-seamonkey2.46: --- → affected
status-seamonkey2.47: --- → affected
(Assignee)

Comment 2

3 years ago
Created attachment 8763336 [details] [diff] [review]
1278130-popupblocking-V1a.patch

oops...
Attachment #8763335 - Attachment is obsolete: true
Attachment #8763335 - Flags: review?(philip.chee)
Attachment #8763336 - Flags: review?(philip.chee)
(Assignee)

Updated

2 years ago
status-seamonkey2.48: --- → affected

Comment 3

2 years ago
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.
Attachment #8763336 - Flags: review?(philip.chee) → review+
(Assignee)

Comment 4

2 years ago
Created attachment 8785597 [details] [diff] [review]
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.
Attachment #8763336 - Attachment is obsolete: true
Attachment #8785597 - Flags: review+
Attachment #8785597 - Flags: approval-comm-beta?
Attachment #8785597 - Flags: approval-comm-aurora?
(Assignee)

Comment 5

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

2 years ago
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
Attachment #8785597 - Flags: approval-comm-beta?
Attachment #8785597 - Flags: approval-comm-beta+
Attachment #8785597 - Flags: approval-comm-aurora?
Attachment #8785597 - Flags: approval-comm-aurora+
(Assignee)

Comment 7

2 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-seamonkey2.46: affected → fixed
status-seamonkey2.47: affected → fixed
status-seamonkey2.48: affected → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.