283108 - [FIX]Eliminate nsIDocShell.parentURIContentListener

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 2

[Darin Fisher]

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.

Comment 3

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

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...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch updated to comments.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated interface comments

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

First patch checked in.

Comment 8

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

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)

Comment 9

[Darin Fisher]

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 :-(

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yeah, I'm just trying to document the mess.  :(

Checked in; marking fixed, since all patches are in.