Stash pointer to the subtree parent on nodes in disconnected subtrees

RESOLVED FIXED in mozilla16

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

Trunk
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox14-, firefox15-)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Being able to always quickly get the subtree parent will make some CC optimizations more performant.
Created attachment 601012 [details] [diff] [review]
Patch
Attachment #601012 - Flags: superreview?(jst)
Attachment #601012 - Flags: review?(bugs)

Comment 2

5 years ago
Comment on attachment 601012 [details] [diff] [review]
Patch


>-  nsIFrame* GetPrimaryFrame() const { return mPrimaryFrame; }
>+  nsIFrame* GetPrimaryFrame() const {
{ should be in the next line.

>+  void SetSubtreeRootPointer(nsINode* aSubtreeRoot) {
ditto

>+  void ClearSubtreeRootPointer() {
And here.

> nsINode::~nsINode()
> {
>   NS_ASSERTION(!HasSlots(), "nsNodeUtils::LastRelease was not called?");
>+  NS_ASSERTION(mSubtreeRoot == this, "Didn't restore state properly?");
> }
Use stronger MOZ_ASSERT here?
Created attachment 601033 [details] [diff] [review]
Patch
Attachment #601012 - Attachment is obsolete: true
Attachment #601033 - Flags: superreview?(jst)
Attachment #601033 - Flags: review?(bugs)
Attachment #601012 - Flags: superreview?(jst)
Attachment #601012 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #2)
> Comment on attachment 601012 [details] [diff] [review]
> Patch
> 
> 
> >-  nsIFrame* GetPrimaryFrame() const { return mPrimaryFrame; }
> >+  nsIFrame* GetPrimaryFrame() const {
> { should be in the next line.

Done.

> >+  void SetSubtreeRootPointer(nsINode* aSubtreeRoot) {
> ditto

Done.

> >+  void ClearSubtreeRootPointer() {
> And here.

Done.

> > nsINode::~nsINode()
> > {
> >   NS_ASSERTION(!HasSlots(), "nsNodeUtils::LastRelease was not called?");
> >+  NS_ASSERTION(mSubtreeRoot == this, "Didn't restore state properly?");
> > }
> Use stronger MOZ_ASSERT here?

It seems odd to have one fatal assert when we use NS_ASSERTION all over the DOM code.

Comment 5

5 years ago
Well, we haven't had MOZ_ASSERT for a long time.
In this case I really want debug builds to crash asap if something goes wrong.

Comment 6

5 years ago
Comment on attachment 601033 [details] [diff] [review]
Patch

>+  nsIFrame* GetPrimaryFrame() const
>+  {
>+    return IsInDoc() ? mPrimaryFrame : nsnull;
I wonder if this shows up in the profiles. Probably not.


>+  union {
I think { should be in the next line.
Attachment #601033 - Flags: review?(bugs) → review+
Comment on attachment 601033 [details] [diff] [review]
Patch

Looks good.
Attachment #601033 - Flags: superreview?(jst) → superreview+
http://hg.mozilla.org/mozilla-central/rev/5280e98d2d77
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Pushed a followup to make OwnerDocAsNode non-inline, with rs=khuey:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/f2bacfc9013c
Version: unspecified → Trunk

Updated

5 years ago
Depends on: 736695
https://hg.mozilla.org/mozilla-central/rev/f2bacfc9013c

Updated

5 years ago
Depends on: 745494
We should consider backing this out of Aurora 14 due to bug 736695.
tracking-firefox14: --- → ?
Although backing this out will probably end up being the solution, we'll instead continue to track bug 736695 since it's the actual regression.
tracking-firefox14: ? → -
Backed out of 14 to fix bug 736695.

https://hg.mozilla.org/releases/mozilla-aurora/rev/abe9bb783f88
Target Milestone: mozilla14 → mozilla15
We need to back this out of Aurora 15 as well....
tracking-firefox15: --- → ?
We have this backout tracked already in bug 736695 so not tracking here.
tracking-firefox15: ? → -
https://hg.mozilla.org/releases/mozilla-aurora/rev/44103bdf65b1
Target Milestone: mozilla15 → mozilla16
And once more, with feeling: https://hg.mozilla.org/releases/mozilla-aurora/rev/0f805ac53831
You need to log in before you can comment on or make changes to this bug.