Closed Bug 211634 Opened 22 years ago Closed 22 years ago

[FIX]Add GetOwnerDocument on nsIContent or nsGenericElement

Categories

(Core :: DOM: Core & HTML, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.5beta

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

Attachments

(1 file)

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.
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...
Attached patch PatchSplinter 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...
Attachment #127842 - Flags: superreview?(jst)
Attachment #127842 - Flags: review?(john)
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).
Priority: -- → P1
Summary: Add GetOwnerDocument on nsIContent or nsGenericElement → [FIX]Add GetOwnerDocument on nsIContent or nsGenericElement
Target Milestone: --- → mozilla1.5beta
Comment on attachment 127842 [details] [diff] [review] Patch 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.
Attachment #127842 - Flags: review?(john) → review+
Comment on attachment 127842 [details] [diff] [review] Patch sr=jst
Attachment #127842 - Flags: superreview?(jst) → superreview+
How come nsINodeInfo::GetDocument isn't inline? Or at least non-virtual?
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 carefully... 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).
Fix checked in.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Component: DOM: Core → DOM: Core & HTML
QA Contact: desale → general
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: