Closed Bug 283108 Opened 20 years ago Closed 20 years ago

[FIX]Eliminate nsIDocShell.parentURIContentListener

Categories

(Core :: DOM: Navigation, defect, P1)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.8beta2

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

Attachments

(2 files, 1 obsolete file)

People who really feel the yen to mess with this can GetInterface an
nsIURIContentListener and set the attribute there (which is equivalent).  Note
that embedders use the frozen nsIWebBrowser.parentURIContentListener, so this
doesn't even affect them; it only affects our internal non-nsWebBrowser-using code.
Attached patch Proposed patch (obsolete) — Splinter Review
Attachment #175096 - Flags: superreview?(darin)
Attachment #175096 - Flags: review?(cbiesinger)
Comment on attachment 175096 [details] [diff] [review]
Proposed patch

>Index: editor/composer/src/nsEditingSession.cpp

>+  // XXXbz this is never unset!  Someone needs to read the idl.

you're gonna file some bugs right? ;-)


>Index: xpfe/browser/resources/content/nsBrowserContentListener.js

>+        if (windowDocShell) {
>+            // XXXbz this listener is never unregistered.  Did someone neglect 
>+            // to read the interface they're using?
>+            windowDocshell.
>+              QueryInterface(Components.interfaces.nsIInterfaceRequestor).
>+              getInterface(Components.interfaces.nsIURIContentListener).
>+                parentContentListener = this;
>+        }

ditto.	also, the trailing dots seem somewhat unconventional to me.
i'm used to seeing leading dots on new lines, but whatever.  just
be consistent with the existing source file, and if you are already
then just ignore me!


>Index: extensions/help/resources/content/help.js
>Index: browser/base/content/browser.js

same comment for these changes.
Attachment #175096 - Flags: superreview?(darin) → superreview+
Comment on attachment 175096 [details] [diff] [review]
Proposed patch

+  // XXXbz this is never unset!  Someone needs to read the idl.

please update nsIURIContentListener.idl to actually mention this...

xpfe/browser/resources/content/nsBrowserContentListener.js (et al)

I also believe the usual style is the dot on the new line...
Attachment #175096 - Flags: review?(cbiesinger) → review+
Attachment #175096 - Attachment is obsolete: true
Assignee: adamlock → bzbarsky
Target Milestone: --- → mozilla1.8beta2
Attachment #175180 - Flags: superreview?(darin)
Attachment #175180 - Flags: review?(cbiesinger)
The composer code is actually ok, since it implements nsISupportsWeakReference
(I removed the XXX comment).  Filed bug 283197, bug 283198, bug 283199 on the JS
files.
First patch checked in.
Priority: -- → P1
Summary: Eliminate nsIDocShell.parentURIContentListener → [FIX]Eliminate nsIDocShell.parentURIContentListener
Comment on attachment 175180 [details] [diff] [review]
Updated interface comments

+  *	   nsnull value

hm... since interface comments are generally js-focussed (@return/@throw etc),
I'd just say "null" here.
(twice)
Attachment #175180 - Flags: review?(cbiesinger) → review+
Comment on attachment 175180 [details] [diff] [review]
Updated interface comments

>Index: uriloader/base/nsIURIContentListener.idl
...
>+  *
>+  * @note If this attribute is set to an object that implements
>+  *       nsISupportsWeakReference, the implementation should get the
>+  *       nsIWeakReference and hold that.  Otherwise, the implementation
>+  *       should not refcount this interface; it should assume that a non
>+  *       nsnull value is always valid.  In that case, the caller is
>+  *       responsible for explicitly setting this value back to nsnull if the
>+  *       parent content listener is destroyed.
>   */
>   attribute nsIURIContentListener parentContentListener;

wow... this is utterly evil.  at least this code gives JS and other
language bindings a fighting chance by trying weak refs first, but
_sigh_... this is so not a valid xpcom setter.	oh well, guess you
can't do much about it since it is frozen :-(
Attachment #175180 - Flags: superreview?(darin) → superreview+
Yeah, I'm just trying to document the mess.  :(

Checked in; marking fixed, since all patches are in.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: