[FIX]Eliminate nsIDocShell.parentURIContentListener

RESOLVED FIXED in mozilla1.8beta2

Status

()

P1
normal
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

Trunk
mozilla1.8beta2
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

14 years ago
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.
(Assignee)

Comment 1

14 years ago
Created attachment 175096 [details] [diff] [review]
Proposed patch
Attachment #175096 - Flags: superreview?(darin)
Attachment #175096 - Flags: review?(cbiesinger)

Comment 2

14 years ago
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+
(Assignee)

Comment 4

14 years ago
Created attachment 175177 [details] [diff] [review]
Patch updated to comments.
Attachment #175096 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Assignee: adamlock → bzbarsky
Target Milestone: --- → mozilla1.8beta2
(Assignee)

Comment 5

14 years ago
Created attachment 175180 [details] [diff] [review]
Updated interface comments
Attachment #175180 - Flags: superreview?(darin)
Attachment #175180 - Flags: review?(cbiesinger)
(Assignee)

Comment 6

14 years ago
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.
(Assignee)

Comment 7

14 years ago
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 9

14 years ago
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+
(Assignee)

Comment 10

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

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