Bug 26140 – Implement onreadystatechange event handler for Documents

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Sam Weinig]

We should implement the onreadystatechange event handler for Documents from HTML5.

[James Simonsen]

Created attachment 70177 [details]
Patch

[Alexey Proskuryakov]

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.

[James Simonsen]

Created attachment 70499 [details]
Patch

[Darin Adler]

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.

[James Simonsen]

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.

[James Simonsen]

Created attachment 70507 [details]
Patch

[Adam Barth]

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?

[Darin Adler]

(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.

[James Simonsen]

Created attachment 70543 [details]
Patch

[James Simonsen]

(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..."

[James Simonsen]

(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().

[Darin Adler]

Comment on attachment 70543 [details]
Patch

I’d like to see even more test cases, but this seems OK to land.

[James Simonsen]

(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.

[Adam Barth]

> 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.

[James Simonsen]

Created attachment 70566 [details]
Patch

[James Simonsen]

(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.

[WebKit Commit Bot]

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

[James Simonsen]

(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?

[Eric Seidel (no email)]

Comment on attachment 70566 [details]
Patch

I suspect it's just a flaky test, not your patch.

[Alexey Proskuryakov]

Is there any evidence of flakiness on the Chromium dashboard or elsewhere? The dashboard doesn't open for me, freezing the browser.

[WebKit Commit Bot]

Comment on attachment 70566 [details]
Patch

Clearing flags on attachment: 70566

Committed r69710: <http://trac.webkit.org/changeset/69710>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.