Code review comment for lp:~rpadovani/oxide/http-status-code

Revision history for this message
Olivier Tilloy (osomon) wrote :

Thanks Riccardo for working on this! The logic looks good and seems to work well. I have a few comments, mostly on the unit tests:

 - In tst_LoadEvent_httpStatusCode.py, the body of the response (html) should be written regardless of the status code (it’s currently written only if http_status_code < 400. Actually, reading the spec more closely, for status codes 204 and 304 there musn’t be a message body, so those need to be special-cased. All others should be fine with a body.

 - In shared/browser/oxide_web_view.cc, the new line after OnLoadRedirected( needs to be indented by 4 whitespaces, for consistency with the rest of the codebase.

 - It should be possible to rewrite the tests in tst_LoadEvent_httpStatusCode.qml to be data-driven (see http://doc.qt.io/qt-5/qml-qttest-testcase.html#data-driven-tests): each row of data would contain a status code, together with a list of expected load events and their respective status codes.

 - Would it be possible to add tests for 3xx and 5xx status codes?

 - What exactly are the issues with the tests for status codes 205 and 407?

« Back to merge proposal