Closed Bug 250342 Opened 21 years ago Closed 20 years ago

Views should not inherit from nsISupports

Categories

(Core :: Web Painting, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: Biesinger, Assigned: Biesinger)

Details

Attachments

(2 files, 2 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
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)
Comment on attachment 152599 [details] [diff] [review] patch we discussed on IRC a better way to do this.
Attachment #152599 - Flags: review?(roc) → review-
Attached patch patch v2 (obsolete) — Splinter Review
uses a new isupports-derived helper class to store the view on the widget.
Attachment #152599 - Attachment is obsolete: true
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.
Attachment #157812 - Attachment is obsolete: true
Attachment #157812 - Flags: superreview?(roc)
Attachment #157812 - Flags: review?(roc)
Attachment #157812 - Flags: review-
Attached patch patch v3Splinter Review
fixes leaks.
Attachment #157863 - Flags: superreview?(roc)
Attachment #157863 - Flags: review?(roc)
Status: NEW → ASSIGNED
Attachment #157863 - Flags: superreview?(roc)
Attachment #157863 - Flags: superreview+
Attachment #157863 - Flags: review?(roc)
Attachment #157863 - Flags: review+
checked in.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Attached patch fix leaksSplinter Review
thanks to dbaron for noticing the extra reference added here
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+
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
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: