404066 - implement webprogress listeners for notificationboxes

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Teune van Steeg]

SeaMonkey's notificationbox should have its own webProgress listener hooked up into its containing browser's webProgress.

Comment 1

[Teune van Steeg]

Attachment: add a progresslistener to the notificationbox

> .removeAllNotifications(true)
seems to work now with this progress listener.

this patch will also remove some abundant whitespace, and it will remove the "*" for notification.xml in jar.mn

Comment 2

[[e-mail]]

Comment on attachment 289075 [details] [diff] [review]
add a progresslistener to the notificationbox

>+              this.activeBrowser.webProgress.addProgressListener(this._filter, Components.interfaces.nsIWebProgress.NOTIFY_ALL);
Two issues:
1. You're not interested in all the progress, only location changes
2. The filter doesn't filter location change notifications, so it's useless

>+      <method name="getProgressListener">
I think it might be possible to implement a progress listener in XBL using implementation implements="nsIObserver, nsIWebProgressListener"

>+              var errorPage = aRequest && !Components.isSuccessCode(aRequest.status);
>+              if (aWebProgress.DOMWindow == this.mBrowser.contentWindow &&
>+                  (aWebProgress.isLoadingDocument || errorPage)) {
>+                this.mNotificationbox.removeAllNotifications(true);
>+              }
I wonder whether we can detect certificate errors and display a notification?
[Separate bug]

>             this._activeBrowser = null;
>             var os = Components.classes["@mozilla.org/observer-service;1"]
>                                 .getService(Components.interfaces.nsIObserverService);
>             try {
>               os.removeObserver(this, "xpinstall-install-blocked");
>             } catch (ex) {}
>+
>+            if (this.activeBrowser && this._progressListener) {
Don't use this.activeBrowser especially after clearing this._activeBrowser!

Comment 3

[Teune van Steeg]

Attachment: add a progresslistener to the notificationbox (v2)

> I wonder whether we can detect certificate errors and display a notification?
> [Separate bug]

I don't know anything about certificates, but maybe it has its own status code (nothing in nsError.h AFAICT)

Comment 4

[Teune van Steeg]

(In reply to comment #3)
> > I wonder whether we can detect certificate errors and display a notification?
> > [Separate bug]
> 
> I don't know anything about certificates, but maybe it has its own status code
> (nothing in nsError.h AFAICT)
> 
or maybe actually
http://mxr.mozilla.org/seamonkey/source/security/nss/lib/base/errorval.c#87

Comment 5

[[e-mail]]

Comment on attachment 289103 [details] [diff] [review]
add a progresslistener to the notificationbox (v2)

>+      <field name="progressListening">false</field>
I would call this either _addedProgressListener or mAddedProgressListener

>+            if (this.activeBrowser && this.progressListening) {
You don't need to check for this.activeBrowser because this.mAddedProgressListener will not be true without an active browser.

>-              // XXX should be: removeAllNotifications(true) once bug 303327 is fixed 
You didn't copy this comment?

Comment 6

[Teune van Steeg]

Attachment: patch (v3)

Rerequesting superreview because you might want to check the following:

(In reply to comment #5)
> >-              // XXX should be: removeAllNotifications(true) once bug 303327 is fixed 
> You didn't copy this comment?
> 
I intended to use removeAllNotifications(true) here, because that works with this progress listener for me. But somehow that patch ended up with removeAllNotifications(false).

Comment 7

[[e-mail]]

Comment on attachment 289227 [details] [diff] [review]
patch (v3)

removeAllNotifications(true) still doesn't work for me :-(

Comment 8

[Teune van Steeg]

Attachment: add a progresslistener to the notificationbox (for check in)

Comment 9

[Robert Kaiser]

Comment on attachment 289231 [details] [diff] [review]
add a progresslistener to the notificationbox (for check in)

checked in this patch per request of Teune via IRC