Closed
Bug 176624
Opened 22 years ago
Closed 20 years ago
Need UI for popup-blocking per-site whitelist
Categories
(SeaMonkey :: UI Design, enhancement)
SeaMonkey
UI Design
Tracking
(Not tracked)
RESOLVED
WORKSFORME
People
(Reporter: dveditz, Assigned: shliang)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
jag+mozilla
:
review+
jag+mozilla
:
superreview+
|
Details | Diff | Splinter Review |
bug 174765 added backend support for a popup-blocking whitelist, now we need the UI.
Attachment #109961 -
Flags: superreview?(dveditz)
Attachment #109961 -
Flags: review?(jaggernaut)
Comment 8•22 years ago
|
||
I would like to see this as well. I have to turn popup blocking off to use an internal tool at my company.
Comment 9•22 years ago
|
||
Comment on attachment 109961 [details] [diff] [review] patch r=jag
Attachment #109961 -
Flags: review?(jaggernaut) → review+
Comment 10•22 years ago
|
||
Comment on attachment 109961 [details] [diff] [review] patch >Index: extensions/cookie/resources/content/pref-popups.xul >=================================================================== > var policyButton; >+ var soundCheckbox; >+ var soundUrlBox; >+ var soundUrlButton; >+ var iconCheckbox; Could you prefix these with "g" for global? >+ function blacklistEmpty() { >+ var permissionmanager = Components.classes["@mozilla.org/permissionmanager;1"] permissionManager (interCaps). >+ while (enumerator.hasMoreElements()) { >+ var nextPermission = enumerator.getNext() This variable should be called |permission|. >+ .QueryInterface(Components.interfaces.nsIPermission); >+ if (nextPermission.type == POPUP_TYPE) { >+ if (!nextPermission.capability) { >+ return false; >+ break; This |break| is unnecessary. Is capability a boolean, or a number you're comparing against? If it's a number, what's the meaning assigned to the value zero? Then again, why are you using permission manager here instead of popup window manager? Danm suspects it's because its getEnumerator() method isn't implemented yet. In that case I suggest you implement it and use that interface. >Index: xpfe/browser/resources/content/navigator.js >=================================================================== >+ var enumerator = permissionmanager.enumerator; >+ var count=0; Fix spacing around =. >+ while (enumerator.hasMoreElements()) { >+ var nextPermission = enumerator.getNext(); Call it permission, and chain the QI to this statement. (or rather, use nsIPopupWindowManager, see earlier comment). >+ if (popupIcon) { Is this null check needed? Don't we always have the popup-icon element? >+ if (!policy) { What does !policy mean? >+ for (var i = 0; i < browsers.length; i++) { >+ if (browsers[i].popupDomain in hosts) Needs another space before |if|. >+ break; >+ browsers[i].popupDomain = null; >+ popupIcon.hidden = true; >+ } >+ } >+ else { The style in navigator.js is to have |else| on the same line as }. >+ for (var i = 0; i < browsers.length; i++) { >+ if (browsers[i].popupDomain in hosts) { >+ browsers[i].popupDomain = null; >+ popupIcon.hidden = true; In both these loops the |popupIcon.hidden = true| line seems misplaced. Shouldn't we only do something with it if a change affects the currently selected tab? >+ } >+ } The above two lines have a lot of whitespace after the }. Didn't check other lines, but please check for that and fix as needed. Could you attach the updated patch, and ask danm for review (for the nsIPopupWindowManager stuff)?
Attachment #109961 -
Flags: superreview?(dveditz)
Attachment #109961 -
Flags: superreview-
Attachment #109961 -
Flags: review-
Attachment #109961 -
Flags: review+
Updated•22 years ago
|
Attachment #111638 -
Flags: superreview?(jaggernaut)
Attachment #111638 -
Flags: review?(danm)
Comment 12•22 years ago
|
||
Comment on attachment 111638 [details] [diff] [review] patch patch review comments: Looks generally fine, but I have two objections. I think the patch is workable despite my objections so I'm giving it the +. Despite that I think these are worth considering: nsPopupEnumerator::GetNext seems kind of large to be an inline method. (Or can a virtual method never be inline? That seems likely but I do want to be certain.) nsPopupEnumerator::HasMoreElements itself iterates over the permission list. Typically it would be used in an iteration loop, where it would be an O(n^2) operation. Is there no more direct way to calculate this? I'm curious why in pref-popups.xul the init() function was explicitly leading-uppercased, when that's not usual JS practice and the other functions in that file are lowercase. I find myself objecting to some of the verbage brought over from the Netscape build. I know. It was approved. But I ask, is the phrase "specify how to handle popup windows that appear on top of or under the current Navigator window" any improvement over "when websites attempt to open windows without being asked"? I find the original verbage much more clear. To my eye, the new approved verbage uses a lot of words to spin around in place. I guess that's my only real verbage objection. Oh. Lied. I noticed an inconsistency. The prefs panel refers to it as "allow" or "block." The popup manager refers to it as "allow" or "suppress." I recommend reconciling the two. My eyes began to glaze over right around popupManager.js. I was still paying attention, but I probably didn't give the most conscientious review from there out.
Attachment #111638 -
Flags: review?(danm) → review+
Updated•22 years ago
|
Attachment #111638 -
Flags: superreview?(jaggernaut) → superreview?(dveditz)
Comment 13•22 years ago
|
||
Comment on attachment 111638 [details] [diff] [review] patch sr=jag, provided danm's comments have been addressed. Oh, and instead of commenting out the allowWindowOpen* stuff in pref-scripts.*, just remove the code.
Updated•22 years ago
|
Attachment #111638 -
Flags: superreview?(dveditz) → superreview+
Comment 15•22 years ago
|
||
Comment on attachment 112129 [details] [diff] [review] final(?) patch Carrying forward r=danm, sr=jag
Attachment #112129 -
Flags: superreview+
Attachment #112129 -
Flags: review+
Comment 16•22 years ago
|
||
>>+ for (var i = 0; i < browsers.length; i++) { >>+ if (browsers[i].popupDomain in hosts) { >>+ browsers[i].popupDomain = null; >>+ popupIcon.hidden = true; > >In both these loops the |popupIcon.hidden = true| line seems misplaced. >Shouldn't we only do something with it if a change affects the currently >selected tab? This was never fixed... I would code it something like this: for (var i = 0; i < browsers.length; i++) if ((browsers[i].popupDomain in hosts) == policy) browsers[i].popupDomain = null; var popupIcon = document.getElementById("popupIcon"); popupIcon.hidden = getBrowser().selectedBrowser.popupDomain == null;
Comment 17•22 years ago
|
||
Is this already supposed to be working? Because AFAICT this is only half working at the moment. Right now I see the new pref UI and can select popup blocking by default. I also get a little symbol in the lower left if a popup is blocked and the popup manager opens when I doubleclick on it. But when I add an adress in the popup manager it doesn't display. While the whitelisting seems to work the popup is not yet fully finshed; it remains empty and doesn't display the whitelisted URLs. Is this another bug or is this bug just not yet completed?
Comment 18•22 years ago
|
||
> it remains empty and doesn't display the whitelisted URLs. > Is this another bug or is this bug just not yet completed? Depending on exactly what you're looking for, that's bug 189960, bug 190014, or bug 190371.
Updated•22 years ago
|
Component: Browser-General → XP Apps: GUI Features
QA Contact: asa → sairuh
Comment 19•22 years ago
|
||
Sorry, but I think that when it comes to popup windows a whitelist is worse than a blacklist: Popups become annoying when they appear and steal the focus again and again on a certain site. Just eventual popup windows must not be blocked because they can contain important information. Peripherally in this sense, Mozilla could become sued for repressing that information. So it should be possible to block only popups from websites explicitly specified. By the way, does Mozilla use the same file or at least the same engine for popup blocking, cookie managing and form auto-completion? As I experienced with the cookie manager, new features are not always implemented in a sleek and sensible way: It opens and closes the cookie file on every access, so if one deletes 100 cookies it opens and closes the file for 100 times (and possibly even issues a flush command to the OS) making my harddisk rattle badly.
Updated•20 years ago
|
Product: Core → Mozilla Application Suite
Comment 20•20 years ago
|
||
There is UI now. I have no idea which bug that added, so marking this wfm :)
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → WORKSFORME
Updated•20 years ago
|
QA Contact: bugzilla → nobody
You need to log in
before you can comment on or make changes to this bug.
Description
•