199705 - add URL of blocked popup to DOMPopupBlocked event

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, enhancement)

Description

[Dan M]

An embeddor has requested we add the URL of the actual blocked popup window to
the DOMPopupBlocked event, so it will be possible to log the blocked windows.
Currently the URL of the window whose attempt to open a popup was blocked is
available from the event target, but not the URL of the popup window itself.

Comment 1

[Dan M]

Attachment: add requesting window's and popup window's URIs to blocked popup DOM event

What, a whole new DOM event? One the W3 has never heard of? I'm told it's the
right thing (tm).

With this patch the popup window's URI is available in the DOMPopupBlocked
event handler. It's accessible like this:

diff -u -r1.498 navigator.js
--- xpfe/browser/resources/content/navigator.js 26 Mar 2003 06:36:13 -00001.498

+++ xpfe/browser/resources/content/navigator.js 10 Apr 2003 20:52:49 -0000
@@ -1998,6 +1998,9 @@
 }

 function onPopupBlocked(aEvent) {
+  var pbe =
aEvent.QueryInterface(Components.interfaces.nsIDOMPopupBlockedEvent);
+  alert("just blocked popup window "+pbe.popupWindowURI.spec);
+
   var playSound = pref.getBoolPref("privacy.popups.sound_enabled");

   if (playSound) {

Comment 2

[Dan M]

Comment on attachment 120111 [details] [diff] [review]
add requesting window's and popup window's URIs to blocked popup DOM event

Johnny: as promised! (Feel free to fight over who gets to be r= and who sr=,
but I'd like jaggernaut, jst and shliang to take a look.)

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 120111 [details] [diff] [review]
add requesting window's and popup window's URIs to blocked popup DOM event

- In nsDOMEvent::InitPopupBlockedEvent():

+  if (mEvent->eventStructType == NS_POPUPBLOCKED_EVENT) {

Shouldn't you assert and return an error if this is not true?

- In nsDOMEvent::GetRequestingWindowURI():

+{
+  NS_ENSURE_ARG_POINTER(aRequestingWindowURI);
+  if (mEvent->eventStructType == NS_POPUPBLOCKED_EVENT) {
+    nsPopupBlockedEvent* event = NS_STATIC_CAST(nsPopupBlockedEvent*, mEvent);
+    *aRequestingWindowURI = event->mRequestingWindowURI;
+    NS_IF_ADDREF(*aRequestingWindowURI);
+  }
+  return NS_OK;

Make sure *aRequestingWindowURI is nulled out if mEvent->eventStructType !=
NS_POPUPBLOCKED_EVENT. Same thing in nsDOMEvent::GetPopupWindowURI().

- In nsEventListenerManager.cpp

-  if (!aEvent && !str.EqualsIgnoreCase("MouseEvents") &&
!str.EqualsIgnoreCase("KeyEvents") &&
-      !str.EqualsIgnoreCase("HTMLEvents") &&
!str.EqualsIgnoreCase("MutationEvents") &&
-      !str.EqualsIgnoreCase("MouseScrollEvents") &&
!str.EqualsIgnoreCase("Events")) {
+  if (!aEvent && !str.EqualsIgnoreCase("MouseEvents") &&
+		  !str.EqualsIgnoreCase("KeyEvents") &&
+		  !str.EqualsIgnoreCase("HTMLEvents") &&		       
		  !str.EqualsIgnoreCase("MutationEvents") &&

Break up the above line too while you're at it :-)

sr=jst

Comment 4

[jag (Peter Annema)]

Comment on attachment 120111 [details] [diff] [review]
add requesting window's and popup window's URIs to blocked popup DOM event

-  if (!aEvent && !str.EqualsIgnoreCase("MouseEvents") &&
!str.EqualsIgnoreCase("KeyEvents") &&
-      !str.EqualsIgnoreCase("HTMLEvents") &&
!str.EqualsIgnoreCase("MutationEvents") &&
-      !str.EqualsIgnoreCase("MouseScrollEvents") &&
!str.EqualsIgnoreCase("Events")) {
+  if (!aEvent && !str.EqualsIgnoreCase("MouseEvents") &&
+		  !str.EqualsIgnoreCase("KeyEvents") &&
+		  !str.EqualsIgnoreCase("HTMLEvents") &&		       
		  !str.EqualsIgnoreCase("MutationEvents") &&
+		  !str.EqualsIgnoreCase("MouseScrollEvents") &&
+		  !str.EqualsIgnoreCase("PopupBlockedEvents") &&
+		  !str.EqualsIgnoreCase("Events")) {

Looks like you missed MutationEvents there.

Comment 5

[Dan M]

Attachment: same as above plus bring the world of NS_ASSERTION home to nsDOMEvent.cpp

Check, assert on NS_POPUPBLOCKED_EVENT; null *aRequestingWindowURI and
*aPopupWindowURI in their accessor methods in case of error. And while I'm at
it, do the same thing to the rest of the file. Now, now it's a big patch.

The !str.EqualsIgnoreCase("MutationEvents") case wasn't actually missing. It
looked that way because it was actually on the same line as the previous
("HTMLEvents") case. Fixed that. I tried breaking up that line by removing some
other cases, like MouseEvents and KeyEvents. But then it didn't work so good.

Some day I'm gonna write a patch that you can't find anything wrong with. It'll
probably be a short one.

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 120463 [details] [diff] [review]
same as above plus bring the world of NS_ASSERTION home to nsDOMEvent.cpp

r/sr=jst

Comment 7

[jag (Peter Annema)]

Comment on attachment 120463 [details] [diff] [review]
same as above plus bring the world of NS_ASSERTION home to nsDOMEvent.cpp

:-)

sr=jag

Comment 8

[Jesse Ruderman]

Does this block bug 198846?

Comment 9

[Dan M]

You'll probably find the information this enhancement makes available useful for
that feature, sure. I've made a note in that bug.

It's been checked in. Works as described in comment 1.