Closed Bug 1278130 Opened 8 years ago Closed 8 years ago

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

Categories

(SeaMonkey :: Passwords & Permissions, defect)

defect
Not set
normal

Tracking

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

RESOLVED FIXED
Tracking Status
seamonkey2.46 --- fixed
seamonkey2.47 --- fixed
seamonkey2.48 --- fixed

People

(Reporter: frg, Assigned: frg)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch 1278130-popupblocking.patch (obsolete) — Splinter Review
A patch! A patch!
Assignee: nobody → frgrahl
Status: NEW → ASSIGNED
Attachment #8763335 - Flags: review?(philip.chee)
Attached patch 1278130-popupblocking-V1a.patch (obsolete) — Splinter Review
oops...
Attachment #8763335 - Attachment is obsolete: true
Attachment #8763335 - Flags: review?(philip.chee)
Attachment #8763336 - Flags: review?(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.
Attachment #8763336 - Flags: review?(philip.chee) → review+
[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?
>> >> 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 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+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: