331194 - "Unblock" should actually show blocked popup, not add site to exceptions list

Status: VERIFIED FIXED

(Camino Graveyard :: General, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Right now, we have two options in the blocked popup code: "Unblock", which adds the site to the exceptions list (whitelist), and "Configure…", which opens the Web Features prefPane to allow the user to open the exceptions list and do whatever.  To actually view a blocked popup, you must first "unblock" it (add to exceptions) and then reload the page (which doesn't make a lot of sense).

"Unblock" implies "show the popup that was blocked", not "add to whitelist"--and when there's a popup, I'd rather look at it first before deciding to whitelist the site, anyway.

"Configure…" seems like an odd option to have, since it just opens the Web Features prefPane, not the exceptions list, so perhaps "Unblock" and "Add to Exceptions List" would be better buttons (and functions) to use.

Comment 1

[Chris Lawson (gone)]

Would the best way to handle this just be to make the "Unblock" button *also* reload the page after it adds the site to the whitelist?

cl

Comment 2

[Samuel Sidler (old account; do not CC)]

There should be a way to do this without reloading the page (as that's what Fx does). I don't think it should add it to the whitelist though, unless we rename the button "unblock and add site to exceptions list" or some other long name.

What Smokey proposed in the third paragraph of comment 0, is what I think we should do.

Comment 3

[Håkan Waara]

Unblock == do not block == whitelist. I think this is logical. If you just want it to show one time, then perhaps "Show" or something else is better.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #3)
> Unblock == do not block == whitelist. 

Unblock = remove the blocking = let through right now.  I don't think "unblock" is universally clear as "whitelist for future visits but don't show right now".

When this was a status bar icon, it would have been nice and relatively easy (UI-wise) to list the blocked popup(s) in the menu so that you could select the window(s) you wanted to show (and Camino would show them) in addition to the other option(s)--which would have made it even easier to know if you'd want to whitelist the site or view the popup at all, given a sane window title--but putting this info/options in the new bar is much more difficult.

Whatever we call them, we really should have "show the blocked popup(s) now" and "whitelist this site for future visits" buttons--and showing the popup should not force a page reload (think about ajax sites and POSTDATA/form submission; reloads aren't going to work, or work right).

Comment 5

[Håkan Waara]

I'll have a try

Comment 6

[Håkan Waara]

Attachment: WIP patch

WIP. Untested, but compiles. This is the backend docshell stuff we'll need to find the blocked popup's docshell.

Now I also have to hook it up to the blocking mechanism, and show the window.

Comment 7

[Håkan Waara]

This patch works fine, and will open all popups you've blocked.

There's just one catch.

The new popups will be blocked by Gecko.

So we're dependent on this Core bug unless we can find out a hack. :-/

Other than that, I've debugged this patch and it seems like it should work fine.

Comment 8

[Håkan Waara]

Attachment: Patch for this

This is the patch. Would probably need to remove a debug statement or two, otherwise it would be final.

Let's see if the DOM guys can fix the blocking bug in a not too distant future, or provide a workaround.

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

In the Core bug 331635, bz mentioned that mfcembed bug, which sounded very similar to a bug we had and pink/geoff/josh fixed: bug 272389.  Is there anything in there that would be helpful here (either as a "workaround" or in fixing the Core bug)?

Comment 10

[Simon Fraser [no longer active]]

Comment on attachment 216148 [details] [diff] [review]
Patch for this

>Index: src/browser/BrowserWindowController.mm
>===================================================================

> - (void)unblockAllPopupSites:(nsISupportsArray*)inSites
> {
>-  nsCOMPtr<nsIPermissionManager> pm (do_GetService(NS_PERMISSIONMANAGER_CONTRACTID));
>-  if (!pm)
>-    return;
>-
>   PRUint32 count = 0;
>   inSites->Count(&count);
>   for (PRUint32 i = 0; i < count; ++i) {
>     nsCOMPtr<nsISupports> genUri = dont_AddRef(inSites->ElementAt(i));

It has nothing to do with genUri any more. And I think that inSites should
be renamed to inBlockedPopupEvents or something.

>+    nsCOMPtr<nsIDOMPopupBlockedEvent> blockedPopup = do_QueryInterface(genUri);
>+    if (!blockedPopup)
>+      return;
>+    
>+    // get the URIs
>+    nsCOMPtr<nsIURI> requestingWindowURI, popupWindowURI;
>+    blockedPopup->GetRequestingWindowURI (getter_AddRefs (requestingWindowURI));

No space before ( please.

>+    blockedPopup->GetPopupWindowURI (getter_AddRefs (popupWindowURI));
>+    
>+    // get the blocked popup's modifiers, and window name
>+    nsAutoString features, windowName;
>+    blockedPopup->GetPopupWindowFeatures (features);
>+    blockedPopup->GetPopupWindowName (windowName);
>+    
>+    // find the docshell for the blocked popup window, in order to show it
>+    nsCOMPtr<nsIDocShell> popupWinDocShell = [[mBrowserView getBrowserView] getDocShellForURI:requestingWindowURI];
>+    if (!popupWinDocShell)
>+      return;
>+    
>+    nsCOMPtr<nsIDOMWindowInternal> domWin = do_GetInterface (popupWinDocShell);
>+    if (!domWin)
>+      return;
>+    
>+    nsCAutoString uriAsString;
>+    popupWindowURI->GetSpec (uriAsString);
>+    
>+    // open the blocked window
>+    nsCOMPtr<nsIDOMWindow> openedWindow;
>+    nsresult rv = domWin->Open (NS_ConvertUTF8toUTF16(uriAsString), windowName, features, getter_AddRefs (openedWindow));

Is this how FF does it?

>+    if (rv != NS_OK)
>+      printf ("wtf!!! %d\n", rv);

Wouldn't you want to factor this window opening code so that you can use it for showing just a single blocked popup?

>Index: src/browser/BrowserWrapper.mm
>===================================================================

>-- (void)onPopupBlocked:(nsIURI*)inURIBlocked fromSite:(nsIURI*)inSite
>+- (void)onPopupBlocked:(nsIDOMEvent*)eventData;
> {
>   // lazily instantiate.
>   if (!mBlockedSites)
>     NS_NewISupportsArray(&mBlockedSites);
>   if (mBlockedSites) {
>-    mBlockedSites->AppendElement(inSite);
>+    mBlockedSites->AppendElement(eventData);
>     [mDelegate showPopupBlocked:YES];
>   }
> }
>@@ -1079,7 +1079,7 @@
> - (void)drawRect:(NSRect)aRect
> {
>   // draw background color
>-  [[NSColor colorWithCalibratedRed:1.0 green:0.9 blue:0.58 alpha:1.0] set];
>+  [[NSColor colorWithCalibratedRed:(199.0/255.0) green:(225.0/255.0) blue:(235.0/255.0) alpha:1.0] set];
>   NSRectFill([self frame]);
>   
>   // draw shadowed border

>Index: src/browser/KeychainService.mm
>===================================================================

>-- (void)onPopupBlocked:(nsIURI*)inURIBlocked fromSite:(nsIURI*)inSite
>+- (void)onPopupBlocked:(nsIDOMEvent*)eventData;

I think you should use nsIDOMPopupBlockedEvent* as the param type.

>Index: src/embedding/CHBrowserView.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/embedding/CHBrowserView.mm,v
>retrieving revision 1.69
>diff -u -r1.69 CHBrowserView.mm
>--- src/embedding/CHBrowserView.mm	4 Mar 2006 16:37:09 -0000	1.69
>+++ src/embedding/CHBrowserView.mm	24 Mar 2006 21:29:45 -0000
>@@ -50,6 +50,11 @@
> #include "nsComponentManagerUtils.h"
> #include "nsIDocCharset.h"
> 
>+// docshell fun (blocked sites)
>+#include "GeckoUtils.h"
>+#include "nsIDocShell.h"
>+#include "nsIDocShellTreeItem.h"
>+
> #include "nsIURI.h"
> #include "nsIDocument.h"
> #include "nsIDOMWindow.h"
>@@ -534,18 +539,8 @@
> // should we be using the window.location URL instead? see nsIDOMLocation.h
> - (NSString*)getCurrentURI
> {
>-  nsCOMPtr<nsIWebNavigation> nav = do_QueryInterface(_webBrowser);
>-  if (!nav)
>-    return @"";
>-
>-  nsCOMPtr<nsIURI> uri;
>-  nav->GetCurrentURI(getter_AddRefs(uri));
>-  if (!uri)
>-    return @"";
>-
>-  nsCAutoString spec;
>-  uri->GetSpec(spec);
>-	return [NSString stringWithUTF8String:spec.get()];
>+  NSString *uriStr = [self getURIForDocShell:[self getDocShell]];\

I think I prefer it the old way.

>+- (NSString*)getURIForDocShell:(nsIDocShell*)aDocShell
>+{
>+	nsCAutoString uriStr;

Bad spacing

>+  GeckoUtils::GetURIForDocShell (aDocShell, uriStr);

No space before (

>+  return [NSString stringWithUTF8String:uriStr.get()];
>+}
>+
>+// used for finding a blocked popup's docshell
>+// addrefs the result!
>+- (already_AddRefed<nsIDocShell>)getDocShellForURI:(nsIURI*)aURI
>+{
>+  nsIDocShell *match;
>+  GeckoUtils::FindDocShellForURI (aURI, [self getDocShell], &match);
>+  return match;
> }
> 
> - (id<CHBrowserContainer>)getBrowserContainer
>Index: src/extensions/GeckoUtils.h
>===================================================================

>+  static void GetURIForDocShell(nsIDocShell* aDocShell, nsACString& aURI)
>+  {
>+    nsCOMPtr<nsIWebNavigation> nav = do_QueryInterface(aDocShell);
>+    nsCOMPtr<nsIURI> uri;
>+    nav->GetCurrentURI(getter_AddRefs(uri));
>+    if (!uri)
>+      aURI.Assign (NS_LITERAL_CSTRING(""));

Better to use Truncate().

>+    else {
>+    	nsCAutoString uriStr;
>+    	uri->GetSpec(uriStr);
>+    	aURI = uriStr;

Why the two steps? uri->GetSpec(aURI); should work fine.

>+  static void FindDocShellForURI (nsIURI *aURI, nsIDocShell *aRoot, nsIDocShell **outMatch)

I think this method is large enough to be moved into a .cpp file. Just make GeckoUtils.cpp and move some of these methods into it.

>+  {
>+    *outMatch = nsnull;
>+    if (!aURI || !aRoot)
>+      return;
>+    
>+    // get the URI we're looking for
>+    nsCAutoString soughtURI;
>+    aURI->GetSpec (soughtURI);
>+    
>+    nsCOMPtr<nsISimpleEnumerator> docShellEnumerator;
>+    aRoot->GetDocShellEnumerator (nsIDocShellTreeItem::typeContent, 
>+                                  nsIDocShell::ENUMERATE_FORWARDS, 
>+                                  getter_AddRefs (docShellEnumerator));
>+    if (!docShellEnumerator)
>+      return;
>+    
>+    PRBool hasMore = PR_FALSE;
>+    nsCOMPtr<nsIDocShellTreeItem> curItem;
>+    
>+    while (NS_SUCCEEDED (docShellEnumerator->HasMoreElements (&hasMore)) && hasMore) {
>+      // get the next element
>+      nsCOMPtr<nsISupports> curSupports;
>+      docShellEnumerator->GetNext (getter_AddRefs (curSupports));
>+      if (!curSupports)
>+        return;
>+      
>+      nsCOMPtr<nsIDocShell> curDocShell = do_QueryInterface (curSupports);
>+      if (!curDocShell)
>+        return;
>+      
>+      // get this docshell's URI
>+      nsCAutoString docShellURI;
>+      GetURIForDocShell (curDocShell, docShellURI);
>+      
>+      if (docShellURI.Equals (soughtURI)) {
>+        // we found it!
>+        NS_ADDREF (*outMatch = curDocShell.get());
>+        return;
>+      }
>+    }
>+    
>+    // We never found the docshell.
>+    return ;

No return statement needed.

>+  }
> };
> 
>

Comment 11

[Håkan Waara]

(In reply to comment #10)

> Is this how FF does it?

Yes, see http://lxr.mozilla.org/seamonkey/source/browser/base/content/browser.js#430

Comment 12

[Håkan Waara]

Attachment: Working patch

This patch works! I'm using the hack Smokey mentioned, like we do in our window-opening code to allow popups.

Changes:

* Added GeckoUtils.cpp
* Removed *a lot* of #includes in GeckoUtils.h and replaced with forward-definitions; now there's only one #include in there!
* Change the mBlockedPopupSites array from deprecated nsISupporsArray to an nsIMutableArray
* Reuse the existing mBlockedPopupSites if it's already allocated, instead of freeing it on every page change.
* Addressed Simon's comments.

Comment 13

[Håkan Waara]

If we want to change the behavior of "Configure..." (or even remove it, in favor of something like "Add to whitelist", as suggested in comment #0), please file a follow-up bug.

Comment 14

[Mark Mentovai]

Comment on attachment 216674 [details] [diff] [review]
Working patch

Note: needs a project file update.

Comment 15

[Håkan Waara]

Comment on attachment 216674 [details] [diff] [review]
Working patch

Mark, Simon:  The changes are quite simpler than they might look. So here's a little review-aid. Just keep it at your side while looking at the changes, and it's possible even a quick one!

>Index: browser/BrowserWindowController.mm
>===================================================================

* unblockAllPopupSites: is called with an array of cached nsIDOMPopupBlockedEvents.

* We iterate through it, and reconstruct (and show) each of the popups given all the info those events contain (which includes title, modifiers, origin etc).

* This is exactly following the logic Firefox uses in browser.js (showBlockedPopup)

>Index: browser/BrowserWrapper.mm
>===================================================================

* BrowserWrapper is the owner of the cached array of blocked popup sites (events). All it does here is clear it on every location change, and add to it when the onPopupBlocked event is called.

* I've changed our usage of nsISupportsArray to nsIArray, since nsISupportsArray is deprecated.

>Index: embedding/CHBrowserView.mm
>===================================================================

* getURIForDocShell: is declared, which is basically a wrapper around a GeckoUtils utility that does all the work. It needs to be defined as already_AddRefed so nsCOMPtrs won't addref the result another time.

>Index: embedding/GeckoUtils.cpp
>===================================================================

* Here, note that 80% of the change is just moved code from previously GeckoUtils.h to GeckoUtils.cpp
* The only changed function is actually ::FindDocShellForURI
* Once again, it's matching a browser.js utility (findChildShell) (12 lines of tidy JS - grr)
* It simply recurses a docshell node, looking for a URI associated with the docshell. Addrefs the result.

That's it!

Comment 16

[Mike Pinkerton (not reading bugmail)]

so how does a user now add the site to the exception list, which is really what they generally want in addition to showing the blocked popup (and the cause of feature requests for a cmd-key to enable/disable popup blocking)?

Comment 17

[Håkan Waara]

(In reply to comment #16)
> so how does a user now add the site to the exception list, which is really what
> they generally want in addition to showing the blocked popup (and the cause of
> feature requests for a cmd-key to enable/disable popup blocking)?

You can do that via the "Configure..." button. But I guess if we come up with a better way, this can be easily fixed in a follow-up bug. (If no one wants it, I can take it.)

Comment 18

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I've added some mockups (and more thoughts) that take Mike's comments into account in bug 333531.

Comment 19

[Håkan Waara]

(In reply to comment #16)
> so how does a user now add the site to the exception list, which is really what
> they generally want in addition to showing the blocked popup (and the cause of
> feature requests for a cmd-key to enable/disable popup blocking)?

If there was actually interest in this patch, one option would be to modify it to make "Unblock" put the site on the whitelist *and* unblock the current site.

Comment 20

[Håkan Waara]

Comment on attachment 216674 [details] [diff] [review]
Working patch

I'll change this to also put on whitelist, at the same time.

Comment 21

[Håkan Waara]

Attachment: Part 1: Create GeckoUtils.cpp part (checked in)

OK, I'm splitting this bug up in parts to make it easier to review this time.

This is part #1. It does this:

* Moves all of current GeckoUtils.h to GeckoUtils.cpp
* Adds FindDocShellForURI() and GetURIForDocShell().

Comment 22

[Håkan Waara]

Comment on attachment 222034 [details] [diff] [review]
Part 1: Create GeckoUtils.cpp part (checked in)

Smfr, wanna r+sr this one?

The only thing that is new are the two functions mentioned in the above comment.

Comment 23

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Trunk pbxproj change (checked in)

Note that GeckoUtils.cpp itself needs a license block; the version in the patch is missing that.

Comment 24

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Branch pbxproj change (checked in)

Gecko changes on the trunk have caused the project files to diverge too far for a reasonable diff to succeed in covering both, so this is the 1.8 branch version of the project patch.

Comment 25

[Håkan Waara]

Comment on attachment 222034 [details] [diff] [review]
Part 1: Create GeckoUtils.cpp part (checked in)

Checked this in. (Mento said smfr's review is sufficient)

Comment 26

[Mark Mentovai]

 15  * The Original Code is mozilla.org code.
 16  *
 17  * The Initial Developer of the Original Code is
 18  * Netscape Communications Corporation.

Really?

Comment 27

[Håkan Waara]

(In reply to comment #26)
>  15  * The Original Code is mozilla.org code.
>  16  *
>  17  * The Initial Developer of the Original Code is
>  18  * Netscape Communications Corporation.
> 
> Really?
> 

Well, the copied code had that license. I added 2 new functions - so what should the license plate be?

Comment 28

[Håkan Waara]

Attachment: Part 2: Hook it up to Camino, updated patch for review (-u8wp)

Here's an updated and tested patch. When you click the "unblock" button, it adds the site to the whitelist and shows the blocked popup(s).

Mark, setting review to you but please punt to someone else if you won't be able to do it.

Overview of the patch:

* Most changes are pretty trivial except for the unblockPopupSites: method. There the array of blocked popup events is "popped". Each popup is shown, and the site is added to the whitelist.

* Remove some unused methods (getBlockedSites:, unblockAllSites:)

Comment 29

[Håkan Waara]

Comment on attachment 226051 [details] [diff] [review]
Part 2: Hook it up to Camino, updated patch for review (-u8wp)

Mark, setting review to you but please punt to someone else if you won't be
able to do it.

See last comment for some review help.

Comment 30

[Mike Pinkerton (not reading bugmail)]

-  nsISupportsArray*         mBlockedSites;    // STRONG
+  nsCOMPtr<nsIMutableArray> mBlockedSites;

the com ptr's ctor and dtor won't be called in an obj-c object, rendering it mostly useless. You're going to have to do the retain/releasing yourself.

looks pretty good otherwise.

Comment 31

[Håkan Waara]

Attachment: Patch v2 (-u8p)

Made the member a nsIMutableArray* instead of a comptr (although note that I did take care to null it out in -dealloc to avoid any leaks).

Comment 32

[Mike Pinkerton (not reading bugmail)]

+#if 0
+// unused?

instead of ifdefing out, why not just remove it? Either that, or do is as part of a separate bug so we don't muddy the waters in this one.

+  nsIMutableArray*          mBlockedSites;

note it's a strong reference

where do you release mBlockedSites? I don't see it in the diff.

Comment 33

[Håkan Waara]

(In reply to comment #32)
> +#if 0
> +// unused?
> 
> instead of ifdefing out, why not just remove it? Either that, or do is as part
> of a separate bug so we don't muddy the waters in this one.

I found that a long time ago and have been running with it since. I'd be glad to just remove it.

> 
> +  nsIMutableArray*          mBlockedSites;
> 
> note it's a strong reference
> 
> where do you release mBlockedSites? I don't see it in the diff.
> 

I don't change the var name, so the NS_IF_RELEASE is still in -dealloc
(http://landfill.mozilla.org/mxr-test/mozilla/source/camino/src/browser/BrowserWrapper.mm#176)

Comment 34

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 227942 [details] [diff] [review]
Patch v2 (-u8p)

ok just remove the code then. 

+#endif
\ No newline at end of file

fix this.

with those changes, sr=pink

Comment 35

[Håkan Waara]

Checked in along with a bunch of other fixes (from froodian/smokey)!

There was a branch bustage (due to nsIMutableArray.idl) not being available, which I fixed by using #ifdef MOZILLA_1_8_BRANCH to include the right thing.

Comment 36

[Håkan Waara]

Attachment: Bustage fix for branch (checked in)

I checked in this fix too, because the nsIDOMPopupBlockedEvent interface lacks the popupWindowName property on the branch.

Spun off bug 343734 for this.