260865 - Remove nsIAlertListener

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

http://lxr.mozilla.org/seamonkey/search?string=wanted%20not

Comment 1

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

do you want to fix other issues in this interface? things that come to mind:
<biesi> 1. can the listener be null
<biesi> 2. can I put up an alert when a previous one is up
<biesi> 3. what happens if I try 2

comments should use doxygen-style

Comment 3

[timeless]

unless there's some real reason for the listener interface, i'd rather it just
use nsIObserver

"put up" should be "post" or something...
"has gone away" should probably be "goes away" or something...

Comment 4

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch

Doesn't touch toolkit.	I'll do that in a separate patch.

Comment 5

[jag (Peter Annema)]

I'll take a look at this later. My first nit to pick would be that I think we
usually define uuids in IDL in all uppercase.

Comment 6

[[e-mail]]

Comment on attachment 163060 [details] [diff] [review]
Patch

>-[scriptable, uuid(7E8B76C6-A70C-4075-81AA-D48B6152E1B4)]
>+[scriptable, uuid(3a3c2953-01ee-4d9c-a102-5aa0d7bd444c)]
Good catch, jag!

>+   gAlertListener = window.arguments[5].QueryInterface(Components.interfaces.nsIObserver);
Nit: This QI shouldn't be necessary because the argument was stored in an
interface pointer which will tell the window watcher what type it is.

>@@ -978,18 +978,24 @@ nsDownload::Observe(nsISupports* aSubjec
>     nsCAutoString path;
>     nsresult rv = GetFilePathUTF8(mTarget, path);
>     // XXX why can't nsDownload cancel itself without help from the dl manager?
>     if (NS_SUCCEEDED(rv))
>       mDownloadManager->CancelDownload(path);
>     // Ignoring return value; this function will get called twice,
>     // and bad things happen if we return a failure code the second time.
>   }
> 
>+  if (strcmp(aTopic, "alertclickcallback") == 0) {
>+    // show the download manager
>+    mDownloadManager->Open(nsnull, this);
>+    return NS_OK;
>+  }
else if? (also in nsMessengerWinIntegration.cpp)

Comment 7

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Revised patch

(In reply to comment #6)
> (From update of attachment 163060 [details] [diff] [review])
> >-[scriptable, uuid(7E8B76C6-A70C-4075-81AA-D48B6152E1B4)]
> >+[scriptable, uuid(3a3c2953-01ee-4d9c-a102-5aa0d7bd444c)]
> Good catch, jag!
Fixed

> >+   gAlertListener =
window.arguments[5].QueryInterface(Components.interfaces.nsIObserver);
> Nit: This QI shouldn't be necessary because the argument was stored in an
> interface pointer which will tell the window watcher what type it is.
I removed the if.  Since we no longer do a QI, if the argument is null, there's
no problem.

> 
> >@@ -978,18 +978,24 @@ nsDownload::Observe(nsISupports* aSubjec
> >	nsCAutoString path;
> >	nsresult rv = GetFilePathUTF8(mTarget, path);
> >	// XXX why can't nsDownload cancel itself without help from the dl
manager?
> >	if (NS_SUCCEEDED(rv))
> >	  mDownloadManager->CancelDownload(path);
> >	// Ignoring return value; this function will get called twice,
> >	// and bad things happen if we return a failure code the second time.
> >   }
> > 
> >+  if (strcmp(aTopic, "alertclickcallback") == 0) {
> >+	// show the download manager
> >+	mDownloadManager->Open(nsnull, this);
> >+	return NS_OK;
> >+  }
> else if? (also in nsMessengerWinIntegration.cpp)

All the ifs return (I added a return statement to the if that was missing one),
and the old code didn't use else if.

Comment 8

[jag (Peter Annema)]

Comment on attachment 163232 [details] [diff] [review]
Revised patch

This looks ok to me. It'd be mighty nice if you could kill those (pre-existing)
tabs in nsIAlertsService.idl while you're poking around in there, and realign
the arguments on the third and fourth line of showAlertNotification.

No need for a new patch for that.

r=jag, requesting sr= from Neil

Comment 9

[[e-mail]]

Comment on attachment 163232 [details] [diff] [review]
Revised patch

>       mDownloadManager->CancelDownload(path);
>     // Ignoring return value; this function will get called twice,
>     // and bad things happen if we return a failure code the second time.
>+
>+    return NS_OK;
Kill that blank line, as the comment refers to the return NS_OK.

I crashed this several times during testing before discovering that the
parameters to showAlertNotification aren't null-checked :-( Someone should
convert the w?strings to AStrings...

Comment 10

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: to check in

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

could you file a new bug about comment 2?

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

filed Bug 267034.

Comment 13

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Filed bug 267089 for toolkit.