Last Comment Bug 211634 - [FIX]Add GetOwnerDocument on nsIContent or nsGenericElement
: [FIX]Add GetOwnerDocument on nsIContent or nsGenericElement
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
P1 normal (vote)
: mozilla1.5beta
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
: Andrew Overholt [:overholt]
Depends on:
  Show dependency treegraph
Reported: 2003-07-03 13:32 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2008-07-31 02:37 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (38.34 KB, patch)
2003-07-15 16:55 PDT, Boris Zbarsky [:bz] (still a bit busy)
john: review+
jst: superreview+
Details | Diff | Splinter Review

Description User image Boris Zbarsky [:bz] (still a bit busy) 2003-07-03 13:32:08 PDT
See comments in bug 211440; the idea is to add a

already_AddRefed<nsIDocument> GetOwnerDocument()

to nsGenericElement (or even nsIContent, depending on what the call patterns
look like...).

I'll do a sweep over the tree looking for GetDocument callers and see whether
there is enough call to put it on nsIContent, but I suspect that it'll be enough
to put it on nsGenericElement for now.
Comment 1 User image Johnny Stenback (:jst, 2003-07-03 14:51:57 PDT
How about we start by putting in on nsGenericElement and if there's enough
callers after that's done, let's take it to nsIContent...
Comment 2 User image Boris Zbarsky [:bz] (still a bit busy) 2003-07-15 16:55:36 PDT
Created attachment 127842 [details] [diff] [review]

One issue arose here.  The leaf HTML classes all decl nsIDOMNode, so they all
declare a local GetOwnerDocument with a different return type and signature. 
Unfortunately, it hides the GetOwnerDocument we want.  Hence the qualification
with the parent class name at those callsites...
Comment 3 User image Boris Zbarsky [:bz] (still a bit busy) 2003-07-15 17:07:00 PDT
Er... the first change in content/base/src/nsImageLoadingContent.cpp should not
be there -- please ignore it (and I will not be checking it in).
Comment 4 User image John Keiser (jkeiser) 2003-07-15 17:46:10 PDT
Comment on attachment 127842 [details] [diff] [review]

Everything here looks fine with the generic "if it compiles" caveat.

There are a few places, such as GetInnerHTML, SetInnerHTML, and some of the
leaf elements, where you replace a simple nodeInfo->GetDocument() with
GetOwnerDocument().  I think this should be safe and probably runs more quickly
(given how cache is accessed) so it's OK with me.  My assumption is that
content's document can never be different than its owner document.  If this
rule can be broken, then you are probably *still* right but I'm not as sure
about the InnerHTML cases.
Comment 5 User image Johnny Stenback (:jst, 2003-07-24 09:30:53 PDT
Comment on attachment 127842 [details] [diff] [review]

Comment 6 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2003-07-24 13:09:41 PDT
How come nsINodeInfo::GetDocument isn't inline? Or at least non-virtual?
Comment 7 User image Boris Zbarsky [:bz] (still a bit busy) 2003-07-24 13:20:17 PDT
I talked to jst about that today, actually.  There are non-layout users of
nsINodeInfo, so inlining and devirtualization would need to be done somewhat

Also, the member lives on nsNodeInfo, not nsINodeInfo.  Which means that to
devirtualize or inline we would first have to merge the two classes (which we
should do).
Comment 8 User image Boris Zbarsky [:bz] (still a bit busy) 2003-07-24 16:24:09 PDT
Fix checked in.

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