Closed Bug 628938 Opened 9 years ago Closed 9 years ago

Treewalker not working properly in Mozilla Treewalker Demo

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0
Tracking Status
blocking2.0 --- final+

People

(Reporter: pleugner, Assigned: craig.topper)

References

()

Details

(Keywords: regression, Whiteboard: [softblocker])

Attachments

(2 files)

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.13) Gecko/20110103 Gentoo Firefox/3.6.13
Build Identifier: Firefox 40b10

If you go to http://www-archive.mozilla.org/docs/dom/samples/treewalkerdemo.xml and press the "All datatypes" button with FF Beta 10, you only get the first type node, while in FF 3 you get all the type nodes in the example XML.

This bug is a follow up from Bug 625722, where Treewalker froze in a demo.

Reproducible: Always

Steps to Reproduce:
1. Go to http://www-archive.mozilla.org/docs/dom/samples/treewalkerdemo.xml
2. Press the "All datatypes" button
Actual Results:  
FF 4 shows only the first datatype node, while it should show all like in FF 3.

Expected Results:  
Show all datatype nodes in the example XML
Assignee: nobody → craig.topper
blocking2.0: --- → final+
Keywords: regression
Whiteboard: [softblocker]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Looking into it. I see at least one problem. Will try out a fix later tonight.
Basic problem is that as we descend into child nodes trying to find one that would be accepted, we don't update "node" to keep track of our descent. So on our way back up we started with the original "node" from before the descent. Causing us to miss siblings of nodes we walked down through.

I have a patch, but I need to run it through the other tests before I upload it.
Attached patch PatchSplinter Review
This point makes "node" always point to a valid node that is update as we move through the tree. This allows us to be able to always find out way back. Previously we were only updating "sibling" as we went down the tree, but it could become null making it impossible to recover. Doing it this way, sibling is no longer needed to be an nsCOMPtr because as soon as we know it's non-null node we copy it to "node" which is an nsCOMPtr.
Attachment #507790 - Flags: review?(jonas)
Attachment #507790 - Attachment is patch: true
Attachment #507790 - Attachment mime type: application/octet-stream → text/plain
This really needs some tests.
Mochitestifying http://www-archive.mozilla.org/docs/dom/samples/treewalkerdemo.xml
would be good.
Comment on attachment 507790 [details] [diff] [review]
Patch

r=me, but we definitely need more tests here. Craig, would you have time to write something comprehensive up?
Attachment #507790 - Flags: review?(jonas) → review+
I'll try to put some tests together this week.
Sorry to be so unpatient, but are there any updates on this? I need this functionality in my addon and it's still not working in the current beta.
We need some tests here before the patch can land.
Peter, if Craig is swamped and you have time to help out by writing the tests, that would probably help a lot!
I'm planning to write a test so if you want to jump into this, Peter, let me know.
Severity: major → normal
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
I've never written tests before for mozilla. What would be required? I have a fulltime job, so I could only do it in the evenings (if my wife doesn't kill me).
(In reply to comment #12)
> I've never written tests before for mozilla. What would be required? I have a
> fulltime job, so I could only do it in the evenings (if my wife doesn't kill
> me).

If you never wrote a test now isn't the best moment but you I invite you to look at the test I'm going to attach and at https://developer.mozilla.org/en/Mochitest if you want to learn.
Attached patch TestsSplinter Review
Attachment #514501 - Flags: review?(Olli.Pettay)
Attachment #514501 - Flags: review?(jonas)
Status: NEW → ASSIGNED
Attachment #514501 - Flags: review?(Olli.Pettay) → review+
Attachment #514501 - Flags: review?(jonas)
Attachment #507790 - Flags: approval2.0?
Attachment #507790 - Flags: approval2.0? → approval2.0+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/8b140daba936
http://hg.mozilla.org/mozilla-central/rev/1c9cd44cff88
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0
Component: DOM: Traversal-Range → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.