Closed Bug 176624 Opened 20 years ago Closed 17 years ago

Need UI for popup-blocking per-site whitelist

Categories

(SeaMonkey :: UI Design, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: dveditz, Assigned: shliang)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

59.03 KB, 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.
Pull the one from Phoenix. It works just fine...
adding blockage for bbird meta, adt1.0.2
Blocks: blackbird
Keywords: adt1.0.2
No longer blocks: blackbird
Blocks: popups
Keywords: adt1.0.2
*** Bug 179073 has been marked as a duplicate of this bug. ***
*** Bug 183152 has been marked as a duplicate of this bug. ***
added cc: myself 
reassigning
Assignee: dveditz → shliang
Attached patch patch (obsolete) — Splinter Review
moving ui over to mozilla
Attachment #109961 - Flags: superreview?(dveditz)
Attachment #109961 - Flags: review?(jaggernaut)
I would like to see this as well. I have to turn popup blocking off to use an
internal tool at my company.
Comment on attachment 109961 [details] [diff] [review]
patch

r=jag
Attachment #109961 - Flags: review?(jaggernaut) → review+
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+
Attached patch patch (obsolete) — Splinter Review
Attachment #109961 - Attachment is obsolete: true
Attachment #111638 - Flags: superreview?(jaggernaut)
Attachment #111638 - Flags: review?(danm)
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+
Attachment #111638 - Flags: superreview?(jaggernaut) → superreview?(dveditz)
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.
Attachment #111638 - Flags: superreview?(dveditz) → superreview+
Attached patch final(?) patchSplinter Review
Attachment #111638 - Attachment is obsolete: true
Comment on attachment 112129 [details] [diff] [review]
final(?) patch

Carrying forward r=danm, sr=jag
Attachment #112129 - Flags: superreview+
Attachment #112129 - Flags: review+
>>+        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;
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?
> 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.
Component: Browser-General → XP Apps: GUI Features
QA Contact: asa → sairuh
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. 
Product: Core → Mozilla Application Suite
There is UI now. I have no idea which bug that added, so marking this wfm :)
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → WORKSFORME
QA Contact: bugzilla → nobody
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.