Closed Bug 109772 Opened 23 years ago Closed 21 years ago

deCOMtaminate nsIView and friends

Categories

(Core :: Web Painting, defect, P1)

Product:
Core
Component:
Web Painting
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.3beta

People

(Reporter: adamlock, Assigned: roc)

References

Details

(Keywords: topembed+, Whiteboard: [dev notes] [whitebox])

Attachments

(4 files, 1 obsolete file)

new nsIView patch
78.03 KB, patch
Details | Diff | Splinter Review
new patch
63.68 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
trivial fix
850 bytes, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
More nsIView cleanup (diff -w)
41.28 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
nsIView & friends should be renamed and disinherited from nsISupports. The "nsI" prefix is causing a great deal of confusion as to the reference counting rules for these interfaces - something that clients should not be required to have special knowledge of at all.
*** Bug 86522 has been marked as a duplicate of this bug. ***
*** Bug 86510 has been marked as a duplicate of this bug. ***
*** Bug 86546 has been marked as a duplicate of this bug. ***
Taking this bug
Assignee: attinasi → kmcclusk
Component: Layout → Views
Target Milestone: --- → Future
batch: adding topembed per Gecko2 document http://rocknroll.mcom.com/users/marek/publish/Gecko/Gecko2Tasks.html
Keywords: topembed
Keywords: topembed → topembed+
Blocks: grouper
Whiteboard: [dev notes]
Whiteboard: [dev notes] → [dev notes] [whitebox]
Priority: -- → P1
Target Milestone: Future → mozilla1.3beta
Hmm, I thought I was planning to do this :-).
I haven't started yet. If you want it. She's all yours :). Do you think you would get to it in 1.3b or 1.4a?
Attached patch fix nsIView API (obsolete) — — Splinter Review
Here's a chunk of work to deCOMtaminate nsIView. Unfortunately nsIView has to stay inheriting from nsISupports, because "widget client data" is assumed to be XPCOM objects and in various places we figure out what kind of data it is by calling QueryInterface. We also use QueryInterface to get from nsIView to nsIScrollableView. However, nsIScrollableView itself doesn't need to inherit from nsISupports, and I've fixed that (and fixed the sites that were illegally doing refcounting on nsIScrollableView). The main work of this patch is to replace the XPCOM-style signatures in nsIView with more rational C++ signatures, and to make the methods inline where appropriate. (I believe that the nsView/nsIView split is still worth having because it reduces compile times by not having to pull in too much to nsIView, and more importantly, maintains a clear distinction between APIs local to the view module and APIs which are usable by layout.) The old XPCOM-style signatures are currently still supported in most cases (a few methods that were unused have been removed completely, a few others have been removed and their call sites updated). Once this patch has been checked in I will seek r+sr-in-advance to go through all the call sites to the XPCOM signatures (many of which are in the view system itself) and convert them to the new APIs, and remove the old signatures and implementations completely. To get this to work I had to change the member variables in nsIView to be of the abstract types (nsIView and nsIViewManager). So when we're in the view system itself we often have to downcast these values to the concrete types nsView and nsViewManager. I redeclared getter methods in nsView to do this conveniently, but to get the nsIViewManager->nsViewManager downcast to work, I had to solve a nasty recursive include problem. nsViewManager.h tells all. I've taken the opportunity to remove GetChildCount and GetChildAt completely and do a couple of other tiny pieces of cleanup.
Comment on attachment 111845 [details] [diff] [review] fix nsIView API Requesting r and sr for this patch, and r+sr-in-advance to convert all callers from the XPCOM style methods to the new methods.
Attachment #111845 - Flags: superreview?(dbaron)
Attachment #111845 - Flags: review?(kmcclusk)
Comment on attachment 111845 [details] [diff] [review] fix nsIView API r=kmcclusk@netscape.com
Attachment #111845 - Flags: review?(kmcclusk) → review+
OS: Windows 2000 → All
Hardware: PC → All
Is renaming nsIView to something non-XPCOMish on the cards too?
I'm not too worried about nsI*. To me it doesn't mean "XPCOM", it just means "abstract interface".
Most people would consider anything starting with nsI* to be an XPCOM interface and following the same ownership rules etc. Leaving it the way it is will confuse a lot of people, whether nsI* means abstract interface or not. Perhaps something like nsA* for abstract just like in the string classes would make the distinction clearer.
I agree with adamlock. Maybe nsAView, nsAFrame?
OK, I'll change the nsIView definition to nsAView, but I'll put in a typedef nsAView nsIView so I can fix up all the users gradually, while I fix up the call sites.
taking
Assignee: kmcclusk → roc+moz
Comment on attachment 111845 [details] [diff] [review] fix nsIView API This patch needs some work to bring it up to the standards of my nsIFrame deCOM. I'll do another iteration.
Attachment #111845 - Attachment is obsolete: true
Attachment #111845 - Flags: superreview?(dbaron)
Here you go. Some slight adjustments. We really need to link gkview into gklayout so we can eliminate some virtual calls where. But we can do that later.
Would it be useful to have an interface with QueryInterface (but not AddRef and Release) that these pseudo-interfaces (nsI*View, nsI*Frame, nsI*Box, nsIScrollPositionListener, nsICompositeListener, etc.) could all inherit from? (I can't think of a good name, though.)
Comment on attachment 113176 [details] [diff] [review] new nsIView patch >Index: view/public/nsIView.h >+ already_AddRefed<nsIViewManager> GetViewManager() const = 0; >+ nsIViewManager* GetViewManagerUnrefed() const { return mViewManager; } Why is one pure virtual and the other inline? This seems to disagree with the |GetViewManager| defined on nsView, and could be very confusing, especially if moving code from someplace that has access to nsView to someplace that doesn't, or vice-versa. Maybe you're better off without the |GetViewManager| above, and renaming |GetViewManagerUnrefed| to |GetViewManager|? >Index: view/src/nsView.cpp > nsView::nsView() > { >- MOZ_COUNT_CTOR(nsView); >- Any reason to remove the counters? More later...
> Maybe you're better off without the |GetViewManager| above, and renaming > |GetViewManagerUnrefed| to |GetViewManager|? I'm planning to upgrade all the sites currently calling nsView::GetViewManager to nsIView::GetViewManagerUnrefed. So this problem will go away and the code will end up being consistent with nsIFrame and hopefully other deCOMtaminated interfaces. > Any reason to remove the counters? Not really. I took them out while I was trying to make nsIView not inherit from nsISupports. I'll put them back. > Would it be useful to have an interface with QueryInterface (but not AddRef > and Release) Yeah. How about nsIQueryable? nsISupports would have to inherit from nsIQueryable. We'd also have to make a rule that when nsIQueryable::QueryInterface returns an interface deriving from nsISupports, it addrefs that interface, but when it returns an interface NOT deriving from nsISupports, it (obviously) does not addref that interface. A little icky perhaps, but it should be OK.
Attached patch new patch — — Splinter Review
This patch does a bunch of cleanup on the way to non-XPCOM nirvana. It's suitable for immediate checkin. Things it does: -- removes nsViewManger::mTransCnt. It was effectively always > 0 and therefore meaningless, doesn't need to be maintained. -- removes nsView::mChildCount. It was only used in trivial ways, doesn't need to be maintained. -- removes a couple of unused functions. -- adds non-XPCOM accessor methods to nsIView so they can be used from layout. -- moves the display list out of nsViewManager::mDisplayList into stack variables, with references passed around as needed. -- Similarly, moves mTranslucentViewCount and mTranslucentArea into stack variables. -- Make sure ShowDisplayList is always compiled in, but just empty if !DEBUG. This could avoid nasty stupid link errors if I accidentally leave in a call to ShowDisplayList in some future checkin. -- Vestigial start of work to move to group opacity model (PUSH_FILTER, POP_FILTER display list elements).
Attachment #125991 - Flags: superreview?(dbaron)
Attachment #125991 - Flags: review?(dbaron)
Comment on attachment 125991 [details] [diff] [review] new patch Why make nsIView::IsRoot virtual? Why not inline it? - autoZIndex = aNode->mView->GetZIndexIsAuto(); + // Hixie says only non-translucent elements can have z-index:auto + autoZIndex = aNode->mView->GetZIndexIsAuto() && aNode->mView->GetOpacity() == 1.0f; Is this the best way to accomplish this? Does something ensure that the z-index is stored as zero when it's auto? Why move the |#ifdef NS_DEBUG| inside of ShowDisplayList? + aView, &aRect, nsnull, displayRootOrigin.x, displayRootOrigin.y, + POP_CLIP, aX - aOriginX, aY - aOriginY, PR_FALSE); If you're going to wrap, could you wrap at under 80 chars?
Attachment #125991 - Flags: superreview?(dbaron)
Attachment #125991 - Flags: superreview+
Attachment #125991 - Flags: review?(dbaron)
Attachment #125991 - Flags: review+
> Why make nsIView::IsRoot virtual? Why not inline it? It's not inlined because the implementation depends on the view manager and I don't want to wrestle with circular includes. It's virtual so that callers from outside gklayout don't fail to link. But none of this matters for performance because it's only called from inside assertions. > Is this the best way to accomplish this? Does something ensure that the > z-index is stored as zero when it's auto? Yes, nsContainerFrame does that. I believe that, according to Hixie, the style system should compute a z-index of 0 if the computed value of 'opacity' is != 1.0, but we may as well ensure it here as well as a line of defense against bugs. Correct operation of the view manager is going to depend on this invariant... > Why move the |#ifdef NS_DEBUG| inside of ShowDisplayList? So that a stray call to ShowDisplayList outside of #ifdef NS_DEBUG doesn't break the build (instead, it will do nothing). > If you're going to wrap, could you wrap at under 80 chars? Yeah, sure. After I check this in I would like to fold this deCOMtamination of nsIView accessors into the pass I'm already doing with nsIFrame. Will you extend your r+sr to cover that?
> After I check this in I would like to fold this deCOMtamination of nsIView > accessors into the pass I'm already doing with nsIFrame. Will you extend your > r+sr to cover that? Sure.
Thank you very much sir! BTW, > the style system should compute a z-index of 0 what I mean is, it should, but currently AFAIK it doesn't.
People, how it happens that i have NS_IMETHODIMP nsView::GetVisibility(nsViewVisibility &aVisibility) const in nsView.cpp while *** GetVisibility() in nsView.h? After last commitments It crashes the, on segment violation. Is it problem with CVS or wrong check-in?
> People, how it happens that i have > NS_IMETHODIMP nsView::GetVisibility(nsViewVisibility &aVisibility) const > in nsView.cpp > while *** GetVisibility() > in nsView.h? There are two versions of the same method: the old one, which takes an nsViewVisiblity& parameter, and the new one, which returns an nsViewVisibility. > It crashes the, on segment violation. Where? Have you got a stack trace?
About crash. I updated mozilla/view from cvs, did make there, then make in mozilla/layout build. So maybe i forgot some place to build, but there is stack crawl on start crash (splash-screen arised, not mozilla windows yet): segment violation occurred nsView::GetVisibility(nsViewVisibility &) const: GetVisibility__C6nsViewR16nsViewVisibility: +000c ed5ffc20: * 0289 movl %eax, (%edx) mozilla-bin:sc frame retaddr fcffb8c0 ed46938f DocumentViewerImpl::MakeWindow(nsIWidget *, nsRect const &) + 0000069f fcffb994 ed466a83 DocumentViewerImpl::InitInternal(nsIWidget *, nsIDeviceContext *, nsRect const &, int, int) + 000001a3 fcffb9e8 ed4661fe DocumentViewerImpl::Init(nsIWidget *, nsIDeviceContext *, nsRect const &) + 0000002a fcffba10 ed0c1482 nsDocShell::SetupNewViewer(nsIContentViewer *) + 00000a12 fcffbc70 ed0bf134 nsDocShell::Embed(nsIContentViewer *, char const *, nsISupports *) + 00000044 fcffbc94 ed0c0303 nsDocShell::CreateContentViewer(char const *, nsIRequest *, nsIStreamListener **) + 000002b3 fcffbcf0 ed0cecff nsDSURIContentListener::DoContent(char const *, int, nsIRequest *, nsIStreamListener **, int *) + 0000011f fcffbd38 ed0d259f nsDocumentOpenInfo::DispatchContent(nsIRequest *, nsISupports *) + 0000089f fcffbf94 ed0d1bf1 nsDocumentOpenInfo::OnStartRequest(nsIRequest *, nsISupports *) + 000000e1 fcffbfd0 ecd24186 nsInputStreamChannel::OnStartRequest(nsIRequest *, nsISupports *) + 0000002a fcffbfec ecd24d05 nsInputStreamPump::OnStateStart(void) + 00000059 fcffc00c ecd24c56 nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream *) + 00000036 fcffc020 ecc89619 nsInputStreamReadyEvent::EventHandler(PLEvent *) + 0000002d fcffc038 ecc4d2cf PL_HandleEvent + 0000001f fcffc050 ecc4d1e7 PL_ProcessPendingEvents + 00000067 fcffc068 ecc4e00b nsEventQueueImpl::ProcessPendingEvents(void) + 0000003b fcffc090 eca9f44b nsAppShell::Run(void) + 0000011b fcffc0b4 ec92abf8 nsAppShellService::Run(void) + 00000024 So if there is some another place to make , or this change is in correspondence with other source changes, please inform me. (Anyway, i "hate" fact that libgkview is static and needs make-ing in layout. I'm working now on BeOS port drawing issues, investigating code in mozilla/view and it is quite time-consuming to see effect of changes in mozilla/view, so if there is way to set it to *.so, it will be very helpful)
The top of the stack is in a file in mozilla/content, so why don't you rebuild that? And, in the future, please don't make comments like that based on partial rebuilds.
What dbaron said. Always do a full build before reporting a bug because there are all sorts of surprising dependencies. I agree that it is annoying that gklayout takes so long to link, but there are benefits to having views and layout all in one big library. And we're going to be taking more advantage of it over time. So there is no way to link views seperately, sorry.
This commit have added a warning on brad TBox: +view/src/nsViewManager.cpp:1167 + `PRInt32 translucentViewCount' might be used uninitialized in this function Indeed, the translucentViewCount variable is ++'ed and then compared to 0 before it is ever initialized!
This patch devirtualizes some more stuff in nsIView. It also removes the IgnoreSetPosition hack. I retested with what seemed to be the testcases that induced the hack and they seem to work. Probably we fixed the underlying problem a while ago. I also changed the specification for GetOffsetFromWidget to be more like what it was documented to be, and renamed it to GetNearestWidget. I think its behavior is much saner and more useful now. Jumping backward and forward between nsIView and nsView is a bit annoying. At some point we may want to merge them. I don't think that's necessary now.
Attachment #135391 - Flags: superreview?(dbaron)
Attachment #135391 - Flags: review?(dbaron)
Comment on attachment 135391 [details] [diff] [review] More nsIView cleanup (diff -w) >Index: view/public/nsIView.h >- nsPoint GetPosition() const { >- // this assertion should go away once we're confident that it's not needed >- NS_ASSERTION(!IsRoot() || (mPosX == 0 && mPosY == 0), >- "root views should always have explicit position of (0,0)"); >- return nsPoint(mPosX, mPosY); >- } >+ nsPoint GetPosition() const { return nsPoint(mPosX, mPosY); } >- nsRect GetBounds() const { >- // this assertion should go away once we're confident that it's not needed >- NS_ASSERTION(!IsRoot() || (mDimBounds.x == 0 && mDimBounds.y == 0), >- "root views should always have explicit position of (0,0)"); >- return mDimBounds; >- } >+ nsRect GetBounds() const { return mDimBounds; } Hmmm. I like assertions. Why not leave them? >+ virtual ~nsIView() {} Is this just to shut up a gcc warning? I think bryner disabled that warning. >Index: view/src/nsView.cpp >- if (nsnull != mWindow) >+ if (mWindow && GetParent()) up to here.
> Hmmm. I like assertions. Why not leave them? Sure, OK. > Is this just to shut up a gcc warning? I think bryner disabled that warning. No. Without this virtual destructor, the destructors for subclasses don't get called and all hell breaks loose.
The reason for the latter is that we do 'delete nsIView*'.
Comment on attachment 135391 [details] [diff] [review] More nsIView cleanup (diff -w) >Index: view/src/nsView.cpp > void nsView::SetPosition(nscoord aX, nscoord aY) > { > mDimBounds.x += aX - mPosX; > mDimBounds.y += aY - mPosY; > mPosX = aX; > mPosY = aY; > >- // XXX Start Temporary fix for Bug #19416 >- if (mShouldIgnoreSetPosition) { >- return; >- } >- // XXX End Temporary fix for Bug #19416 >+ NS_ASSERTION(GetParent() || (aX == 0 && aY == 0), >+ "Don't try to move the root widget to something non-zero"); > >- if (nsnull != mWindow) >+ if (mWindow && GetParent()) Be careful merging this. >+ if (GetParent()) { >+ GetParent()->GetNearestWidget(&offset); >+ } You already null-checked GetParent() earlier in the function; no need to do it again. >+nsIWidget* nsIView::GetNearestWidget(nsPoint* aOffset) >+ if (!v) { >+ if (aOffset) { >+ *aOffset = pt; > } >+ return v->GetViewManager()->GetWidget(); > } Dereferencing v after checking that it is null seems like a bad idea. Up to here: >Index: widget/src/mac/nsDragService.cpp
The only remaining issue is that I'm a little worried about the GetOffsetFromWidget / GetNearestWidget changes. For example, might you end up with a different widget in the nsMenuPopupFrame code? (The newer code seems more correct, though, I guess.) Likewise, the second object frame change seems like it might end up with the wrong widget.
> Dereferencing v after checking that it is null seems like a bad idea. Oh yeah :-). That should be "return GetViewManager()->GetWidget();"
> Likewise, the second object frame change seems like it might end up with the > wrong widget. I think the new code does exactly what the old code used to do: starting with "view" and proceeding up its parent chain, find the first view that has a widget and return that widget into aContainerWidget. The current nsMenuPopupFrame code is weird. Currently, no-one actually sets the widget directly on the viewmanager (I need to rip out all the code to handle that case, actually, it's ugly ugly). So doing GetOffsetFromWidget on the root view either returns null, or (if you're in a non-chrome subdocument) walks up into the containing document looking for the nearest widget. I think the main danger with this change is that never-executed code will come to life and do something unexpected :-) but I seem to remember that menus continued to work... BTW "nscoord viewOffset;" should be "nsPoint viewOffset;" ... I'm not sure how that compiled...
In those cases I was thinking that the old code would not have picked up the root view's widget, but the new could would find it, since GetNearestWidget now checks the view on which it is being called for a widget. But actually, for the nsObjectFrame code, what you did is OK, except that you need to modify aAbs.
> In those cases I was thinking that the old code would not have picked up the > root view's widget Right. I think the old code would mostly just return null for the widget, and now it won't. As you said the new behaviour is probably closer to what the author intended. I guess I should test the new behaviour a bit more. I do modify aAbs, here; + aAbs += viewOffset;
Comment on attachment 135391 [details] [diff] [review] More nsIView cleanup (diff -w) Assuming you've fixed the issues above, and tested the cases where you changed the behavior to what you think the author intended, r+sr=dbaron.
Attachment #135391 - Flags: superreview?(dbaron)
Attachment #135391 - Flags: superreview+
Attachment #135391 - Flags: review?(dbaron)
Attachment #135391 - Flags: review+
Checked in. I'm marking this bug FIXED since nsIView is entirely deCOMtaminated now.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Regarding http://ocallahan.org/mozilla/nsIView-gcc-patch -- I don't think it would fix the egcs bustage since the problem is a link error, and DOM doesn't link against the view library. Essentially, I think what egcs did is that it emitted non-inline copies of all the inline functions in a class with the first real (not inline and not pure virtual) function in the class, but if the class had no real functions, it emitted them in every translation unit. (Newer versions of gcc may well do the same.) This matters because egcs can't inline GetIID -- newer versions of gcc can. I checked in a hacky patch that added an intermediate class with no non-inline non-pure-virtual methods between nsISupports and nsIView where we could add the inline function.
Thanks for explaining that, and fixing the bug
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

Creator:
Created:
Updated:
Size: