Closed
Bug 566416
Opened 15 years ago
Closed 15 years ago
GetFirstChild broken for attribute nodes
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a5
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(1 file)
10.65 KB,
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
See bug 564979 comment 4.
The issue is that we never set mFirstChild for attribute nodes.
At the same time, I would _really_ rather not introduce a virtual function call back in GetFirstChild...
Assignee | ||
Comment 1•15 years ago
|
||
I am now full of well-reasoned hate for this code.
Attachment #445909 -
Flags: review?(jonas)
Comment on attachment 445909 [details] [diff] [review]
How about this?
I pity the fool that touches attribute nodes. That will make all attribute modifications a lot slower from then on out :(
Oh well, even more incentive for us to nuke the whole attribute-node thing.
Attachment #445909 -
Flags: review?(jonas) → review+
Assignee | ||
Comment 3•15 years ago
|
||
> That will make all attribute modifications a lot slower from then on out :(
Until gc, no? I would assume we don't hold strong refs to attribute nodes?
Note that our UI touches attr nodes in some cases; e.g. this code in sessionstore:
1219 Array.forEach(aTab.attributes, function(aAttr) {
1220 if (this.xulAttributes.indexOf(aAttr.name) > -1)
1221 tabData.attributes[aAttr.name] = aAttr.value;
1222 }, this);
(In reply to comment #3)
> > That will make all attribute modifications a lot slower from then on out :(
>
> Until gc, no? I would assume we don't hold strong refs to attribute nodes?
That seems to be an incorrect assumption :(
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDOMAttributeMap.h#163
> Note that our UI touches attr nodes in some cases; e.g. this code in
> sessionstore:
>
> 1219 Array.forEach(aTab.attributes, function(aAttr) {
> 1220 if (this.xulAttributes.indexOf(aAttr.name) > -1)
> 1221 tabData.attributes[aAttr.name] = aAttr.value;
> 1222 }, this);
sounds like we need to get a move on changing the ownership model or otherwise improves things then...
Assignee | ||
Comment 5•15 years ago
|
||
> That seems to be an incorrect assumption :(
Ugh.
Do we want to hold off on this (and go back to having GetFirstChild not use mFirstChild) for now then?
Assignee | ||
Comment 6•15 years ago
|
||
Note that the sessionstore thing is touching chrome, not content, and only touching leaves, so shouldn't slow things down too much.
I guess this is fine to land as-is.
Assignee | ||
Comment 8•15 years ago
|
||
Pushed http://hg.mozilla.org/mozilla-central/rev/19b38b92bda3.
This is tested by tests in the test suite once bug 564979 is checked in.
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•