Closed Bug 404066 Opened 17 years ago Closed 17 years ago

implement webprogress listeners for notificationboxes

Categories

(SeaMonkey :: UI Design, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: twanno, Assigned: twanno)

References

Details

Attachments

(1 file, 3 obsolete files)

SeaMonkey's notificationbox should have its own webProgress listener hooked up into its containing browser's webProgress.
> .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
Attachment #289075 - Flags: superreview?(neil)
Attachment #289075 - Flags: review?(neil)
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!
Attachment #289075 - Flags: superreview?(neil)
Attachment #289075 - Flags: superreview-
Attachment #289075 - Flags: review?(neil)
> 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)
Attachment #289075 - Attachment is obsolete: true
Attachment #289103 - Flags: superreview?(neil)
Attachment #289103 - Flags: review?(neil)
(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 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?
Attachment #289103 - Flags: superreview?(neil)
Attachment #289103 - Flags: superreview+
Attachment #289103 - Flags: review?(neil)
Attachment #289103 - Flags: review+
Attached patch patch (v3) (obsolete) — Splinter Review
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).
Attachment #289227 - Flags: superreview?(neil)
Attachment #289227 - Flags: review+
Attachment #289103 - Attachment is obsolete: true
Comment on attachment 289227 [details] [diff] [review]
patch (v3)

removeAllNotifications(true) still doesn't work for me :-(
Attachment #289227 - Flags: superreview?(neil) → superreview-
Attachment #289227 - Attachment is obsolete: true
Attachment #289231 - Flags: superreview+
Attachment #289231 - Flags: review+
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: