Closed Bug 385750 Opened 13 years ago Closed 13 years ago

Cleanup implementations of QueryInterface in layout/ widget/ and content/

Categories

(Core :: Layout, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: mats, Assigned: mats)

Details

Attachments

(2 files, 2 obsolete files)

Attached patch Patch rev. 1 (obsolete) — Splinter Review
No description provided.
Attached patch Patch rev. 1 (diff -w) (obsolete) — Splinter Review
A couple of notes:
nsDirectionalFrame::QI does not implement nsIFrameDebug/nsIFrame -
is there a reason for this? (are they missing in frame dumps?)
I've left it as is, but perhaps we should call nsFrame::QI at the end?

BTW, do we really need to implement this QI at all?
I could only find one consumer:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsBidiPresUtils.cpp&rev=1.95&mark=598-600#597
couldn't we use NS_STATIC_CAST there instead?


nsImageFrame and nsObjectFrame explicitly implements QI to
nsIFrameDebug/nsIFrame.  I've left it as is, but I think it would be better
if we called its super instead (which will end up in nsFrame::QI).

Let me know if you want any of the above fixed, I'll be happy to do so.
Attachment #269662 - Flags: superreview?(dbaron)
Attachment #269662 - Flags: review?(dbaron)
Did you mean to include the changes outside of layout in the patch for me to review?

r+sr=dbaron on the changes inside layout in that patch, if you fix nsDirectionalFrame, nsImageFrame, and nsObjectFrame to call the base class QI.  And feel free to attach a followup patch on removing the nsDirectionalFrame QI entirely...
(In reply to comment #2)
> Did you mean to include the changes outside of layout in the patch for me to
> review?

Yes, please.

> r+sr=dbaron on the changes inside layout in that patch, if you fix
> nsDirectionalFrame, nsImageFrame, and nsObjectFrame to call the base class 

Ok.  A question regarding QI to nsISupports:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsObjectFrame.cpp&rev=1.596&root=/cvsroot&mark=433-434#414
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsIObjectFrame.h&rev=1.12&root=/cvsroot&mark=54#43
it looks like we currently return a different value than nsFrame::QI
would return - I'm not sure if that is intentional, should I propagate
nsISupports too or leave it as is?
(same thing in nsImageFrame.cpp)
OK, r+sr=dbaron on the widget changes as well.  But I'd prefer if you got module owner review for the content and xpcom changes.

In nsObjectFrame, I think you should propagate the nsISupports QI as well.
Attached patch Patch rev. 2Splinter Review
Attachment #269661 - Attachment is obsolete: true
Attachment #269662 - Attachment is obsolete: true
Attachment #269662 - Flags: superreview?(dbaron)
Attachment #269662 - Flags: review?(dbaron)
Johnny, could you review the content/* changes please?
Attachment #269871 - Flags: superreview?(jst)
Attachment #269871 - Flags: review?(jst)
Comment on attachment 269871 [details] [diff] [review]
Patch rev. 2 (diff -w)

r+sr=jst for the content changes.
Attachment #269871 - Flags: superreview?(jst)
Attachment #269871 - Flags: superreview+
Attachment #269871 - Flags: review?(jst)
Attachment #269871 - Flags: review+
Checked in to trunk at 2007-07-03 19:15 PDT.

-> FIXED
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Summary: Cleanup implementations of QueryInterface within Layout → Cleanup implementations of QueryInterface in layout/ widget/ and content/
Target Milestone: --- → mozilla1.9beta1
On the normal refcounted objects (nsMenuBoxObject and nsDeviceContextSpec*) can't we replace that handwritten code with the NS_IMPL_ISUPPPORTS macros?
(In reply to comment #9)
> On the normal refcounted objects (nsMenuBoxObject and nsDeviceContextSpec*)
> can't we replace that handwritten code with the NS_IMPL_ISUPPPORTS macros?

Filed bug 387211
You need to log in before you can comment on or make changes to this bug.