[webkit-reviews] review requested: [Bug 121123] Web Inspector: Do not try to parse incomplete HTTP requests : [Attachment 211555] Patch addressing Carlos review

[bugzilla-daemon at webkit.org]

Andre Moreira Magalhaes <andrunko at gmail.com> has asked	for review:
Bug 121123: Web Inspector: Do not try to parse incomplete HTTP requests
https://bugs.webkit.org/show_bug.cgi?id=121123

Attachment 211555: Patch addressing Carlos review
https://bugs.webkit.org/attachment.cgi?id=211555&action=review

------- Additional Comments from Andre Moreira Magalhaes <andrunko at gmail.com>
(In reply to comment #6)
> (From update of attachment 211453 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=211453&action=review
> 
> I'm late again, sorry, I have some comments, though, please consider fixing
them in a follow up commit.
> 
np, attach patch should address the issues, see comments below.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:248
> > +	 GSocketClient* client = g_socket_client_new();
> 
> You are leaking this, please use GRefPtr
> 
Done

> > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:249
> > +	 GSocketConnection* connection =
g_socket_client_connect_to_host(client, "127.0.0.1", 2999, NULL,
&error.outPtr());
> 
> Use 0 instead of NULL for the cancellable. You are leaking the connection
too, please use GRefPtr.
> 
Done

> > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:250
> > +	 g_assert(!error.get());
> 
> Use g_assert_no_error instead, that way if there's an error you will see a
message with the error description.
> 
Done

> > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:258
> > +	 g_output_stream_write(ostream, incompleteRequest,
strlen(incompleteRequest), NULL, &error.outPtr());
> 
> Use 0 instead of NULL.
> 
Done

> > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:259
> > +	 g_assert(!error.get());
> 
> Use g_assert_no_error instead, that way if there's an error you will see a
message with the error description.
> 
Done

> > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:264
> > +	 g_input_stream_read_async(istream, response, sizeof(response) - 1,
G_PRIORITY_DEFAULT, NULL, NULL, NULL);
> 
> Use 0 instead of NULL.
> 
Done

> > Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:268
> > +	 // Give a chance for the server to reply.
> > +	 test->wait(2);
> > +	 // If we got any answer it means the server replied to an incomplete
request, lets fail.
> > +	 g_assert(String(response).isEmpty());
> 
> I don't think this is the proper way to test this. You should use a
GAsyncReadyCallback and spin a run loop to get the bytes read with
g_input_stream_read_finish(). You can also start a timeout idle to assert when
reached. With this code we are always waiting for 2 seconds, making running the
tests slower even with it runs correctly. Also you don't need to initialize the
buffer, and then convert it to a String to check if it's empty, just check the
bytes actually read.
Quoting from IRC:
<andrunko> KaL, hi there, so for bug 121123, I will do the changes you
suggested but I am not sure about the last suggestion (do not wait 2 seconds)
<andrunko> the thing is that with the patch (where the test pass) we should
wait for a timeout as expected result
<andrunko> that is why we wait 2 seconds to give a chance to the inspector
server to reply, if it does /not/ reply we have a successful test
<andrunko> if it does reply the test should fail
<KaL> ah, I see, it's the opposite to what I thought

I changed the timeout to 1 second though.

The test still fails without the fix and works with the fix accordingly.

Thanks both for the reviews.