[webkit-reviews] review granted: [Bug 24089] ThreadableLoader::loadResourceSynchronously should do callbacks like the async code. : [Attachment 27961] Proposed fix.

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted David Levin
<levin at chromium.org>'s request for review:
Bug 24089: ThreadableLoader::loadResourceSynchronously should do callbacks like
the async code.
https://bugs.webkit.org/show_bug.cgi?id=24089

Attachment 27961: Proposed fix.
https://bugs.webkit.org/attachment.cgi?id=27961&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
> +    var logElement = document.getElementById('log');
> +    logElement.appendChild(document.createTextNode(message));
> +    logElement.appendChild(document.createElement("p"));

My suggestion was actually a bit different:

  var p = document.createElement("p");
  p.appendChild(document.createTextNode(message));
  logElement.appendChild(p);

But let's keep it as is - regression tests are all about testing unusual coding
practices and edge cases ;)

> +	   Changes the behavior of sync xhr for insecure redirects in two ways:

> +	   + Sends an error event instead of an abort event (which is the same
as async xhr's behavior).
> +	   + Throws a network exception which is what the spec says to do
(http://www.w3.org/TR/XMLHttpRequest/).

Please mention that it's what other browsers do.

> -    changeState(HEADERS_RECEIVED);

Does this happen in didFinishLoading()? I'm worried that this (not just this
change, but refactoring in general) may subtly clash with work Julien Chaffraix
is doing in bug 22475.

r=me