RESOLVED FIXED 220509
[SOUP] Stop using SoupRequest API to load files in preparation for libsoup3 
https://bugs.webkit.org/show_bug.cgi?id=220509
Summary [SOUP] Stop using SoupRequest API to load files in preparation for libsoup3
Carlos Garcia Campos
Reported 2021-01-11 06:30:32 PST
SoupRequest API is gone in libsoup3 and there's no replacement for file requests. GResource and data URI loads already happen in the web process so we only need to care about file and directory loads.
Attachments
Patch (36.64 KB, patch)
2021-01-11 06:41 PST, Carlos Garcia Campos 		aperez: review-
ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (36.38 KB, patch)
2021-01-20 05:15 PST, Carlos Garcia Campos 		aperez: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Carlos Garcia Campos
Comment 1 2021-01-11 06:41:53 PST
Created attachment 417376 [details] Patch
Adrian Perez
Comment 2 2021-01-19 09:00:31 PST
Comment on attachment 417376 [details] 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.
Carlos Garcia Campos
Comment 3 2021-01-20 04:58:38 PST
Comment on attachment 417376 [details] Patch 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 🤷️ Well, we have Variant in WTF, but that would be out of scope of this patch.
Carlos Garcia Campos
Comment 4 2021-01-20 05:15:19 PST
Created attachment 417960 [details] Patch
Adrian Perez
Comment 5 2021-01-25 07:16:19 PST
Comment on attachment 417960 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417960&action=review > Source/WebKit/NetworkProcess/soup/Resources/directory.css:1 > +:root { Could we at least add a comment mentioning that this file comes from libsoup and that therefore it's assumed that it has a license compatible with WebKit's? Other than this detail, I think the patch can be landed.
Carlos Garcia Campos
Comment 6 2021-01-26 03:49:30 PST
Committed r271879: <https://trac.webkit.org/changeset/271879>
Robert Jenner
Comment 7 2021-01-26 16:11:40 PST
Changes in r271879 have caused test: fast/selectors/selection-window-inactive-text-shadow.html to flakey fail. Tracking in: https://bugs.webkit.org/show_bug.cgi?id=221011
Note You need to log in before you can comment on or make changes to this bug.