This is a prerequisite for the XPCOMGC work: currently nsIFrame and therefore all frames inherit from nsISupports. In the new brave world of MMgc, this is really bad, because we will start trying to hold writebarriers to these objects from some places. The attached patch makes nsIFrame inherit from an arbitrary new interface nsFrameQI which supports a variety of QI but doesn't have addref/release or the XPCOM-but-not-really baggage that goes with it. Note that the do_QueryInterface call that take nsFrameQI is not null-safe. When converting old code I errred on the side of caution when I wasn't sure whether a source pointer could be null or not. There are still a few cases of frames that implement real XPCOM interfaces, including: nsComboButtonListener: nsIDOMMouseListener nsSVGPaintServerFrame: nsISVGValue nsComboboxControlFrame: nsIRollupListener nsIsIndexFrame: nsIDOMKeyListener nsMathMLmactionFrame: nsIDOMMouseListener nsSVG*Frame: nsISVGValueObserver nsTableRowGroupFrame: nsILineIteratorNavigator nsAutoRepeatBoxFrame: nsITimerCallback These are going to be unsafe from GC even if they aren't unsafe from XPCOM now, and will file followup to make these some kind of frame proxy object. I am definitely requesting review for mozilla-central and I'd also like an evaluation of whether this would be good to land for 1.9: I think the changed QueryInterface signature without the outparam could be good for perf, but I haven't measured.

Whoops, forgot to hg qrefresh before uploading the patch

So.. I don't like this sort of thing: + nsIScrollableViewProvider* svp = + do_QueryInterface<nsIScrollableViewProvider>(scrollFrame); It has the undesirable property of having two places which specify nsIScrollableViewProvider and the two could get out of sync. We've had exactly this problem with explicit QueryInterface calls. What can we do to get back the automatic type-safety we have on trunk now with do_QI and CallQI? Since we need extra text anyway, I would rather have a decoration on the thing I'm assigning to if that's what's needed to make this work. Why did you make nsIPhonetic no longer scriptable? If we want to eliminate it, we should just do it instead of leaving it around but not usable for its original purpose.

Frankly, I think just using CallQI throughout with it being (as now) a templated thing that inlines to whatever the typesafe thing is might be the simplest thing to do...

Please note that this version of do_QueryInterface is typesafe. You cannot do, for example: nsInlineFrame* inline = do_QueryInterface<nsBlockFrame>(frame); It's unfortunate that we have to specify the template type, but according to the C++ spec you're not allowed to deduce a function template type from only the return type. The existing CallQueryInterface pattern is very non-optimal because it uses an out-parameter, which I've otherwise eliminated from the QI codepath. I made nsIPhonetic no longer scriptable because it doesn't inherit from nsISupports. I didn't remove it entirely because it's used in editor and I didn't want to get into a bigger refactoring game than necessary.

> Please note that this version of do_QueryInterface is typesafe. Ah, ok. That's good to know. > it uses an out-parameter But the inlining optimizes it away, right? And it's shorter to type.... You shouldn't have changed the inheritance of nsIPhonetic, imo. You _should_ have introduced shims that implement it in the frame code. We have a bug on this.

Another thought... Would it be possible to have something more like: do_QueryInterface<nsIScrollableViewProvider> svp(scrollFrame); The assignment operator could do the QI automatically.

Note, this patch was developed on Mac so there's some win/gtk specific (native theming) code that I'll need to fix up as well.

Olli might be interested in helping get rid of the XPCOM interfaces on frames --- they're definitely bad news. I don't know what nsIPhonetic is used for --- anyone? For the frame side at least, it seems GetPhonetic could be moved to nsIFormControlFrame so frames don't have to implement nsIPhonetic. nsILineIterator is a crappy interface and should probably just become non-XPCOM. It is only implemented by frames. The others should be fixed by introducing a proxy object, like we've done in many places to fix crashes/security bugs. So this usage of do_QueryInterface builds on trunk?

Yes, this patch builds and works on trunk mac. Two reftests had off-by-a-pixel errors, but that may just be fonts on my local machine.

(see also bug 385843 about nsIPhonetic)

How about bz's suggestion from comment #6? I'd call it something other than do_QueryInterface though. Should we generalize nsFrameQI a bit in case something else wants to use it? I suspect we may find other needs for it. For example views could use it (although they're going away post-1.9...)

Yes, bz's suggestion will work for at least the initializer cases. Do you want me to do that instead? I kinda wish we didn't because the pointer is just a pointer in debuggers, rather than having to deal with mRealPtr and template functions, which most debuggers get at least kinda wrong.

Call it mRawPtr at least. I'm used to it... I think it's worth not typing the type twice.

ok, this has the wrapper-class changes... it doesn't change the other bits like nsIPhonetic, because I'd prefer not to make additional changes in this patch unless absolutely necessary.

What do you think about generalizing (the names of) nsFrameQI and nsFramePtr so that we can use them in other contexts?

In terms of automatic rewriting I'd prefer other contexts to be separate: that way we know what ownership model is in force. What are the other proposed contexts anyway?

Views is one, although we'll remove those for Mozilla2 anyway. I suspect as we GC more and refcount less, we'll find more situations where we still want to QI. But this is OK. I guess later if we want to we could make nsFrameQI and nsFramePtr inherit from some common base nsQI or something. After I review this, are you going to do another round with the non-Mac stuff and ask for review on that separately? or what?

Yes I will, I have this as an mqueue so I can put up the remaining bits separately... turns out there's some accessibility wonkiness too.

This is still changing things so that one can no longer use nsIPhonetic on an editor from script anymore. At which point, it'll basically be unusable and might as well be removed entirely, no?

Comment on attachment 281746 [details] [diff] [review] Updated, still mac-only jorendorff gave me a tip on how to make the do_QueryInterface magic work. I'm going to do that and submit a complete rollup patch.

(In reply to comment #20) Well, admittedly I couldn't keep my mouth shut since there was a C++ puzzle on the table... but in the long run, I would rather avoid depending on this kind of magic. I think we stand to lose something from having so many "cute tricks" in our code. But I guess that's really a separate discussion.

This is the completed patch with the jorendorff magic, ready for review (for mozilla-central)

Overloading the same function name (do_QueryInterface) that we use for nsCOMPtr seems like a bad idea. I'm also concerned about the size of this patch if it lands on Mozilla 2 and not 1.9. It could have been done significantly more conservatively -- e.g., without changing the signature of QueryInterface. I think if this patch lands on one and not the other, it will require significant layout expertise to do some of the merging necessary. Since I don't think anybody with that layout expertise is currently working on Mozilla 2, or will be for quite a while, I think it significantly increases the chances that the fork will be permanent.

It is not possible to do this patch correctly while keeping the same signature for QueryInterface: I have to keep the COM QI impls entirely separate from the frame QI impls, in order for the next step of automatic rewriting to work correctly. Actually I should probably change the name of nsFrameQI::QueryInterface altogether because the COM QI signature may change and then we'd have matching signature again. It is theoretically possible to write an overloaded CallQueryInterface signature: template<class D> void CallQueryInterface(nsFrameQI* source, D* *destination) { *destination = static_cast<D*>(source->QueryInterface(NS_GET_TEMPLATE_IID(D))); } And when this is inlined it could end up the same as the direct version. Frankly unless layout is planning significant frame-hierarchy changes for 1.9(.1) I don't think the merge conflicts from this would be difficult at all; the callsite changes are very straightforward: the only place where things really get tricky is the implementations of COMQI/FrameQI.

There's not going to be a layout fork, and we are deCOMtaminating soon. Talking about a fork is not productive. The fact is we'll need layout experise on Mozilla 2 soon. If anyone thinks we should make major pre-Mozilla-2 changes to layout, please mail me. /be

roc/dbaron, I'd like some guidance on how to proceed. I can rename QueryInterface->QueryFrame and do_QueryInterface->do_QueryFrame fairly easily, but I don't want to make any further changes to this (not autogenerated) patch without guidance as to what will be acceptable.

New names are probably good for everyone, but as I mentioned, I would like names that aren't frame-specific in case we switch other classes from refcounting to GC that need some kind of queryinterface capability. I'm not worried about the difficulty of merges, just their quantity. How about this: Land a minimal patch with compatible CallQueryInterface for mozilla2 now. Defer further changes (new names and elimination of CallQueryInterface) until as late as possible. Does that sound reasonable?

We need at least private-repo-hosted deCOMtamination patches that are aggressive and may cause merge hardships for some folks, but these need not be permanent if taras's tools can be rerun to generate new patches. Right now, we really want to get working builds, codesighs results, and perf/footprint results. So no one should panic about a big merge tax. We will diverge at some point, but two thoughts: 1. Rerun taras's tools when necessary against trunk to make a less conflicty patch, apply that (i.e., use patch, not hg merging). 2. Share the merge work. It does not all require layout gurus, all the time, for all hunks in conflict. We really want to get upper-bound codesighs wins, so some amount of hacking should be done very soon, even if it results in leaky or buggy code. E.g., systematic sresult removal (out param becomes return value) plus exception handling, measure codesigh, note outstanding bugs, remove patch and iterate on tooling (for RAII and other exception safety). I'll be back at work this week and poking around to help get such numbers. All this means that no one should panic about merge conflicts. But, we do need a coordinated roadmap for how to develop post-1.9 without forking into two major lines of development, while making good use of hg.mozilla.org and trading off merge conflicts and progress on Mozilla 2 well. I'll be working on that too. On a more generic name than QueryFrame, some candidates: Query (too short and overloaded!) QueryBase QueryAbstractBase QueryAbstaction QueryForIID (yechh!) DynamicCast I'm currently in favor of the last one, simply by hating the others more. /be

Please note that this patch is not auto-generated As I've continued to develop stuff like this, I think the base class should be frame-specific. It is possible/likely that we can use integers to identify frames in the future, instead of expensive IIDs. I think we should use separate base classes for each ownership model. Thus my increasingly strong preference for QueryFrame

Okay then, I accept that.

(In reply to comment #30) > Okay then, I accept that. Whew! Spares us from DynamicCast. :-) /be

Comment on attachment 282613 [details] [diff] [review] Complete, all platforms, rev. 1 >diff -r 45182e442e21 -r b03452777fd4 layout/generic/nsFrame.cpp >--- a/layout/generic/nsFrame.cpp Tue Sep 25 20:49:12 2007 -0400 >+++ b/layout/generic/nsFrame.cpp Thu Sep 27 18:32:00 2007 -0400 >@@ -5078,15 +5043,11 @@ nsIFrame::GetFrameFromDirection(nsDirect ... >- nsISupports *isupports = nsnull; >- result = frameTraversal->CurrentItem(&isupports); >- if (NS_FAILED(result)) >- return result; >- if (!isupports) >+ traversedFrame = frameTraversal->CurrentItem(); >+ if (!traversedFrame) > return NS_ERROR_NULL_POINTER; > //we must CAST here to an nsIFrame. nsIFrame doesn't really follow the rules > //for speed reasons >- traversedFrame = (nsIFrame *)isupports; You should probably remove the "//we must CAST..." comment here (I never quite understood it). >diff -r 45182e442e21 -r b03452777fd4 layout/generic/nsSelection.cpp >--- a/layout/generic/nsSelection.cpp Tue Sep 25 20:49:12 2007 -0400 >+++ b/layout/generic/nsSelection.cpp Thu Sep 27 18:32:00 2007 -0400 >@@ -2061,14 +2060,11 @@ nsFrameSelection::GetFrameFromLevel(nsIF ... >- result = frameTraversal->CurrentItem(&isupports); >- if (NS_FAILED(result)) >- return result; >- if (!isupports) >+ foundFrame = frameTraversal->CurrentItem(); >+ if (!foundFrame) > return NS_ERROR_NULL_POINTER; > //we must CAST here to an nsIFrame. nsIFrame doesn't really follow the rules > //for speed reasons >- foundFrame = (nsIFrame *)isupports; Same here.

I agree with bsmedberg. Ideally we would have an enum with the various frame types which could include both frame "interfaces" as well as concrete frame classes. That should make things clearer and faster.

This is the new work in progress... it's a lot cleaner than the old one, and the dependent work has been moved into other bugs which can land first. It appears to basically run on Linux, though I haven't done thorough unit-test runs on it yet, and it's likely to break on mac/windows in platform-specific accessibility and menu code.

>+class nsQueryFrameResult >+{ >+public: >+ nsQueryFrameResult(nsQueryFrame *s) : mRawPtr(s) { } >+ >+ template<class Dest> >+ operator Dest*() { >+ if (!mRawPtr) >+ return nsnull; >+ >+ return reinterpret_cast<Dest*>(mRawPtr->QueryFrame(Dest::kFrameIID)); >+ } >+ >+private: >+ nsQueryFrame *mRawPtr; >+}; >+ >+inline nsQueryFrameResult do_QueryFrame(nsQueryFrame *f) >+{ >+ return nsQueryFrameResult(f); >+} Might as well cut out the middle man and name the class do_QueryFrame ;-)

This patch builds on all three platforms (at least in local testing). I'm having trouble getting meaningful reftests currently (a totally clean build fails hundreds of them on Linux, so I'm going to try mac a bit later)... but this should be ready for review.

Comment on attachment 346532 [details] [diff] [review] nsQueryFrame for review, rev. 1 - return layoutObject->QueryInterface(NS_GET_IID(nsITableLayout), - (void**)(tableLayoutObject)); + *tableLayoutObject = do_QueryFrame(layoutObject); + return NS_OK; This is possibly a behaviour change --- slightly safer to "return *tableLayoutObject ? NS_OK : NS_NOINTERFACE;" - nsresult rv = NS_ERROR_FAILURE; while (frame && - frame->IsFrameOfType(nsIFrame::eLineParticipant) && - NS_FAILED(rv)) { + frame->IsFrameOfType(nsIFrame::eLineParticipant) && !mBlockFrame) { frame = frame->GetParent(); - rv = frame->QueryInterface(kBlockFrameCID, (void**)&mBlockFrame); + mBlockFrame = do_QueryFrame(frame); } - NS_ASSERTION(NS_SUCCEEDED(rv) && mBlockFrame, "Cannot find containing block."); + NS_ASSERTION(mBlockFrame, "Cannot find containing block."); You need to set mBlockFrame to nsnull before entering the loop. It should probably be a do-while... +#ifndef nsQueryFrame_h__ +#define nsQueryFrame_h__ C++ fascists tell me that __ is prohibited anywhere in the identifier. I guess nsQueryFrameh_h_ works +class nsQueryFrame +{ +public: + enum FrameIID { + BRFrame_id, + CanvasFrame_id, + nsAreaFrame_id, + nsAutoRepeatBoxFrame_id, I love the smell of brute force in the morning! Otherwise fabulous.

Pushed to m-c. http://hg.mozilla.org/mozilla-central/rev/54b5c634212e Appears to have stuck without any tinderbox orangeness

Whoops. That was for a different bug.

It looks like this bug's fix was recently pushed as http://hg.mozilla.org/mozilla-central/rev/61ca56673468 However, this seems to break compilation of nsLayoutDebuggingTools.cpp (on my local Ubuntu machine, in my debug build) with errors including: ../../../dist/include/xpcom/nsISupportsUtils.h:203: error: ‘class nsIFrame’ has no member named ‘QueryInterface’ ./../../dist/include/xpcom/nsISupportsUtils.h:203: error: no class template named ‘COMTypeInfo’ in ‘class nsIFrameDebug’ I'm attaching the relevant chunk of the compile log, for reference. Perhaps this patch should be backed out?

Is nsLayoutDebuggingTools not part of the default debug config? Porting should be straightforward and I'll have a patch up shortly.

Nope, I just realized it's not -- I think you need this in your .mozconfig: ac_add_options --enable-extensions=default,layout-debug

Main patch: http://hg.mozilla.org/mozilla-central/rev/61ca56673468 Static analysis and graphing code: http://hg.mozilla.org/mozilla-central/rev/54cf0cbe42e8 Layout-debugger fixup: http://hg.mozilla.org/mozilla-central/rev/52be68be1534 Except for a pre-existing windows crashtest failure, this stuck with no test failures.

V.Fixed, per bug 391976 comment 6.

Could this land on 1.9.1 branch too ? (To fix bug 458844 there too.)

No.

This is mentioned here: https://developer.mozilla.org/en/Firefox_3.6_for_developers#Other_interface_changes