[webkit-reviews] review denied: [Bug 22475] REGRESSION: Async XMLHttpRequest never finishes on nonexistent files anymore : [Attachment 27895] New version updated with ap's comments

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 22475: REGRESSION: Async XMLHttpRequest never finishes on nonexistent files
anymore
https://bugs.webkit.org/show_bug.cgi?id=22475

Attachment 27895: New version updated with ap's comments
https://bugs.webkit.org/attachment.cgi?id=27895&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
Test results don't match expectations here: readyState 4 is only logged once.
Also, I'm pretty sure that interleaving of the two requests' results is timing
dependent and unreliable. You need waitUntilDone()/notifyDone(), and it would
be better to start the second request once the first is done (or to normalize
the results in some other way, e.g. by logging into separate DIVs).

> +    // For compatibility we fake a correct load for local files in error
(see bug 22475).
> +    // We do not dispatch a readyState event for LOADING to match the normal
local file handling.
> +    if (m_url.isLocalFile()) {

Should this say "file: URLs" instead of local files? For one thing, these can
be directories, and they can be on remote server volumes that are mounted
locally. And yes, I'm unhappy about the existing method name, too.

I've performed some testing now, and I wonder why this is limited to local
files at all. I've tried loading an http resource from a non-existent domain
(to make sure I get a network error, not a 404 response), and both Safari 3 and
Firefox 3 dispatched readystate events for states 2 and 4 - and ToT WebKit
didn't. Unless I did something wrong in my test, this fix should not be limited
to local files.