[webkit-reviews] review denied: [Bug 60875] [SOUP] Abnormal operation if server sends 5xx status code without HTTP body : [Attachment 95734] add test case on LayoutTests/http/tests/loading

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Keunsoon Lee
<keunsoon.lee at samsung.com>'s request for review:
Bug 60875: [SOUP] Abnormal operation if server sends 5xx status code without
HTTP body
https://bugs.webkit.org/show_bug.cgi?id=60875

Attachment 95734: add test case on LayoutTests/http/tests/loading
https://bugs.webkit.org/attachment.cgi?id=95734&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=95734&action=review

Sorry for the late review. This looks pretty good, but I have a few nits. See
below.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:408
> +static bool soupErrorAndHaveNotReceivedBody(SoupMessage* message)

I think this could be renamed to something like:
receivedClientOrServerErrorWithoutBody.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:434
> +    if ((message && SOUP_STATUS_IS_TRANSPORT_ERROR(message->status_code)) 
> +	   || soupErrorAndHaveNotReceivedBody(message)) {

Please make this one line.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:491
> +	       if (d->m_cancelled || !(client = handle->client())) {
> +		   cleanupSoupRequestOperation(handle.get());
> +		   return;
> +	       }

I'm not sure I understand why you are resetting client here. Why is that? Can
the client change?

> LayoutTests/http/tests/loading/status-code-error-without-response-body.html:4

> +This test checks how FrameLoader acts if status code error (i.e. 4xx and
5xx) is come with no response body.<br>

Should read "This test verifies that FrameLoader properly handles 4xx and 5xx
errors without response bodies."

> LayoutTests/http/tests/loading/status-code-error-without-response-body.html:5

> +You can see series of PASS and DONE, if success.<br><br>

Should read "If this test succeeds, you should see a series of "PASS" messages
and then "DONE""

>
LayoutTests/http/tests/loading/status-code-error-without-response-body.html:49
> +var curStatusCode = 0;
> +var curTest = 0;

Should be currentStatusCode and currentTest.

>
LayoutTests/http/tests/loading/status-code-error-without-response-body.html:70
> +    if (curTest < tests.length) {
> +	   if (curStatusCode < statusCodes.length) {
> +	       runTest(tests[curTest][0]+statusCodes[curStatusCode++],
tests[curTest][1]);
> +	   } else {
> +	       curStatusCode = 0;
> +	       curTest++;
> +	       nextTest();
> +	   }
> +    } else {
> +	    done();
> +    }

Typically in WebKit we use early returns. So this would be something like:

if (currentTest >= tests.length) {
    done();
   return;
}

if (currentStatusCode >= statusCodes.length) {
    runTest(tests[currentTest][0] + statusCodes[currentStatusCode++],
tests[currentTest][1]);
    return;
}

 currentStatusCode = 0;
 currentTest++;
 nextTest();

>
LayoutTests/http/tests/loading/status-code-error-without-response-body.html:79
> +    log("It is only useful inside the regression test tool.\n");

You should probably mention DumpRenderTree here explicitly. "This test needs to
be run in DumpRenderTree."