[webkit-reviews] review denied: [Bug 220509] [SOUP] Stop using SoupRequest API to load files in preparation for libsoup3 : [Attachment 417376] Patch

Tue Jan 19 09:00:31 PST 2021

Adrian Perez <aperez at igalia.com> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 220509: [SOUP] Stop using SoupRequest API to load files in preparation for
libsoup3
https://bugs.webkit.org/show_bug.cgi?id=220509

Attachment 417376: Patch

https://bugs.webkit.org/attachment.cgi?id=417376&action=review

--- Comment #2 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 417376
  --> https://bugs.webkit.org/attachment.cgi?id=417376
Patch

Patch looks good overall, but I have a few comments, could you please take
a look at them below? Thanks!

View in context: https://bugs.webkit.org/attachment.cgi?id=417376&action=review

> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:259
> +	   else if (m_file) {

I checked the code a bit, and IIUC it looks like only one of m_inputStream,
m_multipartInputStream, m_soupRequest, or m_file can be valid at a time. It
would be nice to convert this to use std::variant<…> at some point — but of
course that's out of the scope of this patch, and we would need to build in
C++17 mode first ��️

> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1182
> +    if (file != task->m_file.get())

As far as I can see, this could be an ASSERT(file == task->m_file.get())
because the callback is only used above (in line 247) where m_file.get()
is passed to g_file_query_info_async().

The only case in which m_file.get() could be different is after a call
to ::clearRequest(), as done below, but at this point here the pointer
should always be valid because the request has not been cleared yet.

> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1200
> +    g_file_read_async(file, RunLoopSourcePriority::AsyncIONetwork,
task->m_cancellable.get(),
reinterpret_cast<GAsyncReadyCallback>(readFileCallback),
protectedThis.leakRef());

This will try to read the file even if g_file_query_info_finish() fails.
Do I understand correctly that the idea is to try to read the file anyway
and let it fail, to avoid duplicating the code to handle read errors here?
If that's the case, a one-line comment explaining this will be handy for
whoever needs to read this code in the future.

> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1222
> +    if (file != task->m_file.get())

Same here, this could be an ASSERT().

> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1247
> +    if (file != task->m_file.get())

And here, this could be an ASSERT() as well.

> Source/WebKit/NetworkProcess/soup/Resources/directory.css:1
> +:root {

Does this CSS file also come from the Firefox directory listing code?
At least we should have a comment here stating the license for this file.

> Source/WebKit/NetworkProcess/soup/WebKitDirectoryInputStream.cpp:46
> +	   "<html><head>"						   \

The backslashes at the end of line are not needed, the preprocessor will
happily paste strings together into a single one across multiple lines.

> Source/WebKit/NetworkProcess/soup/WebKitDirectoryInputStream.cpp:101
> +	   "<tr>"							   \

Same here, the backslashes are not needed.