176624 - Need UI for popup-blocking per-site whitelist

Status: RESOLVED WORKSFORME

(SeaMonkey :: UI Design, enhancement)

Description

[Daniel Veditz [:dveditz]]

bug 174765 added backend support for a popup-blocking whitelist, now we need the UI.

Comment 1

[Carl Schrader]

Pull the one from Phoenix. It works just fine...

Comment 2

[Michael Buckland]

adding blockage for bbird meta, adt1.0.2

Comment 3

[Jason Bassford]

*** Bug 179073 has been marked as a duplicate of this bug. ***

Comment 4

[R.K.Aa.]

*** Bug 183152 has been marked as a duplicate of this bug. ***

Comment 5

[Christian Schweer]

added cc: myself 

Comment 6

[shliang]

reassigning

Comment 7

[shliang]

Attachment: patch

moving ui over to mozilla

Comment 8

[Lorin Rivers]

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

[jag (Peter Annema)]

Comment on attachment 109961 [details] [diff] [review]
patch

r=jag

Comment 10

[jag (Peter Annema)]

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)?

Comment 11

[shliang]

Attachment: patch

Comment 12

[Dan M]

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.

Comment 13

[jag (Peter Annema)]

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.

Comment 14

[shliang]

Attachment: final(?) patch

Comment 15

[jag (Peter Annema)]

Comment on attachment 112129 [details] [diff] [review]
final(?) patch

Carrying forward r=danm, sr=jag

Comment 16

[[e-mail]]

>>+        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

[Simon Spiegel]

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

[Jason Bassford]

> 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.

Comment 19

[Ulrich Grassberger]

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. 

Comment 20

[Michiel van Leeuwen (email: mvl+moz@)]

There is UI now. I have no idea which bug that added, so marking this wfm :)