Closed
Bug 283108
Opened 20 years ago
Closed 20 years ago
[FIX]Eliminate nsIDocShell.parentURIContentListener
Categories
(Core :: DOM: Navigation, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla1.8beta2
People
(Reporter: bzbarsky, Assigned: bzbarsky)
Details
Attachments
(2 files, 1 obsolete file)
11.55 KB,
patch
|
Details | Diff | Splinter Review | |
3.14 KB,
patch
|
Biesinger
:
review+
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
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•20 years ago
|
||
Attachment #175096 -
Flags: superreview?(darin)
Attachment #175096 -
Flags: review?(cbiesinger)
Comment 2•20 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 3•20 years ago
|
||
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•20 years ago
|
||
Attachment #175096 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Assignee: adamlock → bzbarsky
Target Milestone: --- → mozilla1.8beta2
Assignee | ||
Comment 5•20 years ago
|
||
Attachment #175180 -
Flags: superreview?(darin)
Attachment #175180 -
Flags: review?(cbiesinger)
Assignee | ||
Comment 6•20 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•20 years ago
|
||
First patch checked in.
Priority: -- → P1
Summary: Eliminate nsIDocShell.parentURIContentListener → [FIX]Eliminate nsIDocShell.parentURIContentListener
Comment 8•20 years ago
|
||
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•20 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•20 years ago
|
||
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.
Description
•