[Webkit-unassigned] [Bug 60875] [SOUP] Abnormal operation if server sends 5xx status code without HTTP body

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=60875


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #95734|review?                     |review-
               Flag|                            |




--- Comment #13 from Martin Robinson <mrobinson at webkit.org>  2011-06-15 14:02:44 PST ---
(From update of attachment 95734)
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."

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.