Bug 38219 - Add test for NodeIterator prototype change behavior
: Add test for NodeIterator prototype change behavior
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Other Mac OS X 10.5
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-27 14:14 PDT by Adam Barth
Modified: 2010-04-27 14:51 PDT (History)
2 users (show)

See Also:


Attachments
Patch (3.30 KB, patch)
2010-04-27 14:15 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (3.90 KB, patch)
2010-04-27 14:43 PDT, Adam Barth
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2010-04-27 14:14:00 PDT
Add test for NodeIterator behavior change from removing custom bindings
Comment 1 Adam Barth 2010-04-27 14:15:12 PDT
Created attachment 54455 [details]
Patch
Comment 2 Eric Seidel 2010-04-27 14:23:41 PDT
Comment on attachment 54455 [details]
Patch

A script-test is better:

script-tests/node-iterator-prototype.js:

description("This test checks whether DOM wrappers created by NodeIterator have their prototypes attached to the correct objects.  These nodes are from the child frame, so that's where their prototypes should be attached.")

function runTest() {
    shouldBeEqualToString("it.nextNode().foo", "child");
    shouldBeEqualToString("it.previousNode().foo", "child");
}

Node.prototype.foo = "parent"

var iframe = document.createElement("iframe");
iframe.onload = "runTest()";
frame.src = "resources/node-iterator-prototype-frame.html";
document.body.appenChild(iframe);


Also.  How do we know that nextNode() and previousNode() are not previously acceessed and thus cached (thus making this test a no-op?)  Did you make sure it failed in an earlier version of Safari?

Why doesn't this need to be waitUntilDone?
Comment 3 Adam Barth 2010-04-27 14:28:26 PDT
> A script-test is better:

The problem is we need to wait for the iframe to load.  Can a script test do that?

> Also.  How do we know that nextNode() and previousNode() are not previously
> acceessed and thus cached (thus making this test a no-op?)

I'm pretty sure nextNode() is fresh.  However, I bet previousNode() is cached.  I'm not familiar enough with NodeIterator's API to know how to "skip over" a node so that I can previousNode() back to it and have it be fresh.

> Did you make sure it failed in an earlier version of Safari?

I'll check.

> Why doesn't this need to be waitUntilDone?

Because the load event for the main frame waits for the load event of a the child iframes and the testing harness waits for the main frame's load event to fire before ending the test.
Comment 4 Adam Barth 2010-04-27 14:29:51 PDT
Hum...  It passes in release Safari.
Comment 5 Eric Seidel 2010-04-27 14:32:48 PDT
JSC bindings are cached on a per-window basis iirc.  (you would know better than I).  So if the node has ever been accessed as a binding in any frame it will have a cached binding object with the global object from when it was created.  To test this kind of code we have to be careful to access fresh nodes as you say.  There is no way to clear the cache to my knowledge.
Comment 6 Adam Barth 2010-04-27 14:40:20 PDT
I can get it to fail if I just iterate some more nodes.  The sad part is that it show the bug isn't fixed.  :(
Comment 7 Adam Barth 2010-04-27 14:43:58 PDT
Created attachment 54457 [details]
Patch
Comment 8 Eric Seidel 2010-04-27 14:47:03 PDT
Comment on attachment 54457 [details]
Patch

Wow!  That's awesome.  Thanks.

Would still be better if your test output PASS or FAIL instead of/in addition to parents vs. child.  It's not clear reading the expected output that we're wrong.
Comment 9 Adam Barth 2010-04-27 14:51:39 PDT
Committed r58339: <http://trac.webkit.org/changeset/58339>