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

VERIFIED FIXED in Camino1.5

Status

Camino Graveyard
General
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: Smokey Ardisson (offline for a while; not following bugs - do not email), Assigned: Håkan Waara)

Tracking

(Depends on: 1 bug, {fixed1.8.1})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1
Dependency tree / graph

Details

(URL)

Attachments

(5 attachments, 4 obsolete attachments)

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.
Target Milestone: --- → Camino1.1

Comment 1

11 years ago
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
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.
(Assignee)

Comment 3

11 years ago
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.
(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).
(Assignee)

Comment 5

11 years ago
I'll have a try
Assignee: mikepinkerton → hwaara
(Assignee)

Comment 6

11 years ago
Created attachment 215934 [details] [diff] [review]
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.
(Assignee)

Comment 7

11 years ago
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.
(Assignee)

Updated

11 years ago
Depends on: 331635
(Assignee)

Comment 8

11 years ago
Created attachment 216148 [details] [diff] [review]
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.
Attachment #215934 - Attachment is obsolete: true
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

11 years ago
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.

>+  }
> };
> 
>
(Assignee)

Comment 11

11 years ago
(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
(Assignee)

Comment 12

11 years ago
Created attachment 216674 [details] [diff] [review]
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.
Attachment #216148 - Attachment is obsolete: true
Attachment #216674 - Flags: superreview?(sfraser_bugs)
Attachment #216674 - Flags: review?(mark)
(Assignee)

Comment 13

11 years ago
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

11 years ago
Comment on attachment 216674 [details] [diff] [review]
Working patch

Note: needs a project file update.
(Assignee)

Comment 15

11 years ago
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!
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)?
(Assignee)

Comment 17

11 years ago
(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.)
I've added some mockups (and more thoughts) that take Mike's comments into account in bug 333531.
OS: Mac OS X 10.2 → Mac OS X 10.3
QA Contact: general
(Assignee)

Comment 19

11 years ago
(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.
(Assignee)

Comment 20

11 years ago
Comment on attachment 216674 [details] [diff] [review]
Working patch

I'll change this to also put on whitelist, at the same time.
Attachment #216674 - Attachment is obsolete: true
Attachment #216674 - Flags: superreview?(sfraser_bugs)
Attachment #216674 - Flags: review?(mark)
(Assignee)

Comment 21

11 years ago
Created attachment 222034 [details] [diff] [review]
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().
Attachment #222034 - Flags: review?(mark)
(Assignee)

Comment 22

11 years ago
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.
Attachment #222034 - Flags: review?(mark) → review?(sfraser_bugs)

Updated

11 years ago
Attachment #222034 - Flags: review?(sfraser_bugs) → review+
Created attachment 223239 [details] [diff] [review]
Trunk pbxproj change (checked in)

Note that GeckoUtils.cpp itself needs a license block; the version in the patch is missing that.
Created attachment 223240 [details] [diff] [review]
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.
(Assignee)

Comment 25

11 years ago
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)
Attachment #222034 - Attachment description: Part 1: Create GeckoUtils.cpp part → Part 1: Create GeckoUtils.cpp part (checked in)
(Assignee)

Updated

11 years ago
Attachment #223239 - Attachment description: Project patch to add GeckoUtils.cpp (trunk) → Trunk pbxproj change (checked in)
(Assignee)

Updated

11 years ago
Attachment #223240 - Attachment description: Project patch to add GeckoUtils.cpp (1.8branch) → Branch pbxproj change (checked in)

Comment 26

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

Really?
(Assignee)

Comment 27

11 years ago
(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?
(Assignee)

Comment 28

11 years ago
Created attachment 226051 [details] [diff] [review]
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:)
Attachment #226051 - Flags: review?
(Assignee)

Comment 29

11 years ago
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.
Attachment #226051 - Flags: review? → review?(mark)
-  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.
(Assignee)

Comment 31

11 years ago
Created attachment 227942 [details] [diff] [review]
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).
Attachment #226051 - Attachment is obsolete: true
Attachment #227942 - Flags: review?(mikepinkerton)
Attachment #226051 - Flags: review?(mark)
+#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.
(Assignee)

Comment 33

11 years ago
(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 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
Attachment #227942 - Flags: review?(mikepinkerton) → superreview+
(Assignee)

Comment 35

11 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
(Assignee)

Comment 36

11 years ago
Created attachment 228283 [details] [diff] [review]
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.
(Assignee)

Updated

11 years ago
Blocks: 343734

Updated

11 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.