Bug 26140 - Implement onreadystatechange event handler for Documents
Summary: Implement onreadystatechange event handler for Documents
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-02 10:43 PDT by Sam Weinig
Modified: 2010-10-13 16:11 PDT (History)
5 users (show)

See Also:


Attachments
Patch (5.04 KB, patch)
2010-10-07 17:37 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (8.73 KB, patch)
2010-10-11 17:20 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (9.19 KB, patch)
2010-10-11 18:28 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (7.29 KB, patch)
2010-10-12 11:14 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (9.37 KB, patch)
2010-10-12 15:05 PDT, James Simonsen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2009-06-02 10:43:23 PDT
We should implement the onreadystatechange event handler for Documents from HTML5.
Comment 1 James Simonsen 2010-10-07 17:37:45 PDT
Created attachment 70177 [details]
Patch
Comment 2 Alexey Proskuryakov 2010-10-07 23:14:20 PDT
The test case looks quite weak. You may want to check the test cases we have for readystatechange in XMLHttpRequest - I suspect that this may give ideas of what to test.

My understanding is that this has been supported by MSIE for a long time, and Firefox 4 also has it now. With this history, there must be lots of non-trivial cases to check for.
Comment 3 James Simonsen 2010-10-11 17:20:52 PDT
Created attachment 70499 [details]
Patch
Comment 4 Darin Adler 2010-10-11 17:23:57 PDT
Comment on attachment 70499 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70499&action=review

> WebCore/dom/Document.cpp:1900
> -void Document::implicitOpen()
> +void Document::implicitOpen(bool removeExistingEventListeners)
>  {
>      cancelParsing();
>  
> +    if (removeExistingEventListeners)
> +        removeAllEventListeners();

Why is adding a boolean better than calling removeAllEventListeners in Document::open?

> WebCore/dom/Document.h:536
> -    void implicitOpen();
> +    void implicitOpen(bool removeExistingEventListeners);

This seems like a poor change. Normally if we are just passing in a boolean constant we use an enum instead so it’s named and can be understood at the call site. But in this case, it seems the caller can just call removeAllEventListeners.
Comment 5 James Simonsen 2010-10-11 17:31:32 PDT
Thanks for the quick reply!

(In reply to comment #4)
> (From update of attachment 70499 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70499&action=review
> 
> > WebCore/dom/Document.cpp:1900
> > -void Document::implicitOpen()
> > +void Document::implicitOpen(bool removeExistingEventListeners)
> >  {
> >      cancelParsing();
> >  
> > +    if (removeExistingEventListeners)
> > +        removeAllEventListeners();
> 
> Why is adding a boolean better than calling removeAllEventListeners in Document::open?

The spec for document.open() says to cancel parsing before removing event listeners. I don't know how critical the order is here though.
Comment 6 James Simonsen 2010-10-11 18:28:42 PDT
Created attachment 70507 [details]
Patch
Comment 7 Adam Barth 2010-10-11 18:33:07 PDT
Comment on attachment 70507 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70507&action=review

> WebCore/dom/Document.cpp:990
> +    dispatchEvent(Event::create(eventNames().readystatechangeEvent, false, false));

Does this really dispatch synchronously?
Comment 8 Darin Adler 2010-10-11 20:34:47 PDT
(In reply to comment #5)
> The spec for document.open() says to cancel parsing before removing event listeners. I don't know how critical the order is here though.

I don’t imagine there is anything detectable about the order, but I could be wrong. If there is something detectable we should write a test case demonstrating it.
Comment 9 James Simonsen 2010-10-12 11:14:04 PDT
Created attachment 70543 [details]
Patch
Comment 10 James Simonsen 2010-10-12 11:14:22 PDT
(In reply to comment #7)
> (From update of attachment 70507 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70507&action=review
> 
> > WebCore/dom/Document.cpp:990
> > +    dispatchEvent(Event::create(eventNames().readystatechangeEvent, false, false));
> 
> Does this really dispatch synchronously?

Yeah, it says "fire a new event" as opposed to "queue a task to fire..."
Comment 11 James Simonsen 2010-10-12 11:15:29 PDT
(In reply to comment #8)
> (In reply to comment #5)
> > The spec for document.open() says to cancel parsing before removing event listeners. I don't know how critical the order is here though.
> 
> I don’t imagine there is anything detectable about the order, but I could be wrong. If there is something detectable we should write a test case demonstrating it.

Yeah, I can't figure out any way for it to matter, so I moved it to open().
Comment 12 Darin Adler 2010-10-12 11:45:10 PDT
Comment on attachment 70543 [details]
Patch

I’d like to see even more test cases, but this seems OK to land.
Comment 13 James Simonsen 2010-10-12 11:57:57 PDT
(In reply to comment #12)
> (From update of attachment 70543 [details])
> I’d like to see even more test cases, but this seems OK to land.

I'll gladly add more, but I'm not really sure what other cases to cover. I'm still new, so I could use some ideas for additional test cases if you have them.
Comment 14 Adam Barth 2010-10-12 12:10:53 PDT
> I'll gladly add more, but I'm not really sure what other cases to cover. I'm still new, so I could use some ideas for additional test cases if you have them.

In particular, I'd like to see some test cases around the synchroniscity of the event dispatch.  Also, you might consider what the readystate / event handlers look like some strange execution contexts, like after a document.write inside an <iframe src="about:blank" onload="..."> handler.
Comment 15 James Simonsen 2010-10-12 15:05:42 PDT
Created attachment 70566 [details]
Patch
Comment 16 James Simonsen 2010-10-12 15:06:50 PDT
(In reply to comment #14)
> > I'll gladly add more, but I'm not really sure what other cases to cover. I'm still new, so I could use some ideas for additional test cases if you have them.
> 
> In particular, I'd like to see some test cases around the synchroniscity of the event dispatch.  Also, you might consider what the readystate / event handlers look like some strange execution contexts, like after a document.write inside an <iframe src="about:blank" onload="..."> handler.

Alright, I added 3 more cases:
- Make sure event is synchronous.
- IFrame onload.
- Parent document destroyed.
Comment 17 WebKit Commit Bot 2010-10-12 17:24:10 PDT
Comment on attachment 70566 [details]
Patch

Rejecting patch 70566 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2
Last 500 characters of output:
s successful.
Files=14, Tests=304,  1 wallclock secs ( 0.74 cusr +  0.18 csys =  0.92 CPU)
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Projects/CommitQueue/LayoutTests
Testing 21535 test cases.
http/tests/appcache/fail-on-update-2.html -> failed

Exiting early after 1 failures. 20546 tests run.
442.93s total testing time

20545 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
32 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/4385001
Comment 18 James Simonsen 2010-10-13 11:52:39 PDT
(In reply to comment #17)
> (From update of attachment 70566 [details])
> http/tests/appcache/fail-on-update-2.html -> failed

I synced and reran this locally on Mac and Linux. I can't reproduce it. Looking at the test, it doesn't look like it should be sensitive to anything I changed. Is there any chance this is flakiness?
Comment 19 Eric Seidel (no email) 2010-10-13 12:14:08 PDT
Comment on attachment 70566 [details]
Patch

I suspect it's just a flaky test, not your patch.
Comment 20 Alexey Proskuryakov 2010-10-13 12:17:41 PDT
Is there any evidence of flakiness on the Chromium dashboard or elsewhere? The dashboard doesn't open for me, freezing the browser.
Comment 21 WebKit Commit Bot 2010-10-13 16:11:06 PDT
Comment on attachment 70566 [details]
Patch

Clearing flags on attachment: 70566

Committed r69710: <http://trac.webkit.org/changeset/69710>
Comment 22 WebKit Commit Bot 2010-10-13 16:11:12 PDT
All reviewed patches have been landed.  Closing bug.