Closed
Bug 250342
Opened 21 years ago
Closed 20 years ago
Views should not inherit from nsISupports
Categories
(Core :: Web Painting, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: Biesinger, Assigned: Biesinger)
Details
Attachments
(2 files, 2 obsolete files)
11.29 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
832 bytes,
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
Since views are not refcounted, they should not inherit from nsISupports, to
make it clear that they aren't.
The patch is surprisingly simple (much more pleasant than the nsIFrame
equivalent...)
hmm, or so I thought until I noticed:
209 aWidget->GetClientData(clientData);
210 nsISupports* data = (nsISupports*)clientData;
211
212 if (data) {
213 nsIView* view = nsnull;
214 if (NS_SUCCEEDED(data->QueryInterface(NS_GET_IID(nsIView), (void
**)&view))) {
http://lxr.mozilla.org/seamonkey/source/view/src/nsView.cpp#209
hmm. nsISupports has QueryInterface as the first function. so maybe this would
actually work :)
I tried to do this a while ago and ran into the same problem. Good luck.
Assignee | ||
Comment 2•21 years ago
|
||
well, this works. this does not crash in either the case where that widget data
is an view nor where it isn't.
is this too hackish? (to rely on the fact that queryinterface is for both
interfaces the first virtual function)
Assignee | ||
Updated•21 years ago
|
Attachment #152599 -
Flags: review?(roc)
Comment on attachment 152599 [details] [diff] [review]
patch
we discussed on IRC a better way to do this.
Attachment #152599 -
Flags: review?(roc) → review-
Assignee | ||
Comment 4•20 years ago
|
||
uses a new isupports-derived helper class to store the view on the widget.
Assignee | ||
Updated•20 years ago
|
Attachment #152599 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #157812 -
Flags: superreview?(roc)
Attachment #157812 -
Flags: review?(roc)
Comment on attachment 157812 [details] [diff] [review]
patch v2
Looks good. But in nsView::SetWidget I think you should destroy the old wrapper
if there is one. And nsView::LoadWidget leaks the wrapper if CallCreateInstance
fails.
Assignee | ||
Updated•20 years ago
|
Attachment #157812 -
Attachment is obsolete: true
Attachment #157812 -
Flags: superreview?(roc)
Attachment #157812 -
Flags: review?(roc)
Attachment #157812 -
Flags: review-
Assignee | ||
Comment 6•20 years ago
|
||
fixes leaks.
Assignee | ||
Updated•20 years ago
|
Attachment #157863 -
Flags: superreview?(roc)
Attachment #157863 -
Flags: review?(roc)
Assignee | ||
Updated•20 years ago
|
Status: NEW → ASSIGNED
Attachment #157863 -
Flags: superreview?(roc)
Attachment #157863 -
Flags: superreview+
Attachment #157863 -
Flags: review?(roc)
Attachment #157863 -
Flags: review+
Assignee | ||
Comment 7•20 years ago
|
||
checked in.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 8•20 years ago
|
||
thanks to dbaron for noticing the extra reference added here
Assignee | ||
Updated•20 years ago
|
Attachment #157907 -
Flags: superreview?(dbaron)
Attachment #157907 -
Flags: review?(dbaron)
Attachment #157907 -
Flags: superreview?(dbaron)
Attachment #157907 -
Flags: superreview+
Attachment #157907 -
Flags: review?(dbaron)
Attachment #157907 -
Flags: review+
Assignee | ||
Comment 9•20 years ago
|
||
leak fix checked in
Checking in src/nsView.cpp;
/cvsroot/mozilla/view/src/nsView.cpp,v <-- nsView.cpp
new revision: 3.195; previous revision: 3.194
done
Updated•6 years ago
|
Component: Layout: View Rendering → Layout: Web Painting
You need to log in
before you can comment on or make changes to this bug.
Description
•