RESOLVED FIXED 220764
[SOUP] Stop using SoupRequest API in preparation for libsoup3
https://bugs.webkit.org/show_bug.cgi?id=220764
Summary [SOUP] Stop using SoupRequest API in preparation for libsoup3
Carlos Garcia Campos
Reported 2021-01-20 05:18:33 PST
It's gone in libsoup3, we can just use soup_session_send_async() instead, which is what SoupRequest does internally.
Attachments
Patch (37.10 KB, patch)
2021-01-20 05:33 PST, Carlos Garcia Campos
aperez: review+
aperez: commit-queue-
Patch for landing (36.88 KB, patch)
2021-01-26 03:55 PST, Carlos Garcia Campos
no flags
Patch for landing (36.88 KB, patch)
2021-01-26 04:33 PST, Carlos Garcia Campos
no flags
Carlos Garcia Campos
Comment 1 2021-01-20 05:33:17 PST
Adrian Perez
Comment 2 2021-01-25 13:45:06 PST
Comment on attachment 417962 [details] Patch Patch LGTM with a couple of nits. Please check them out before landing, in particular I would like to see the suggested simplification for ”m_acceptEncoding” in the final version that lands; the other two suggestions I will leave them up to you =) View in context: https://bugs.webkit.org/attachment.cgi?id=417962&action=review > Source/WebCore/platform/network/soup/ResourceRequest.h:85 > + bool m_acceptEncoding : 1; It does not make sense to specify a bit width of “1” here. This is the only member for which the bit width is indicated, which will result in the compiler adding padding anyway—which defeats the intent of saving some bytes of memory. We may as well do: bool m_acceptEncoding { true }; …and skip setting the initial value in the constructors above. Just tested: % cat > t.cc <<EOF #include <cstdio> enum Flag { FLAG_FOO, FLAG_BAR, FLAG_BAZ }; class X { bool bit: 1; enum Flag f; }; class Y { bool bit; enum Flag f; }; int main() { std::printf("X: %zu bytes, Y: %zu bytes\n", sizeof(X), sizeof(Y)); } EOF % g++ -o t t.cc && ./t X: 8 bytes, Y: 8 bytes % > Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:-219 > - Nice to see this bit gone \o/ > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:333 > + g_object_set_data_full(G_OBJECT(task->m_pendingResult.get()), "wk-send-request-data", protectedData.release(), [](gpointer data) { Is there any reason not to use “g_object_set_qdata_full()”? That would avoid the string→GQuark conversion on each usage. Maybe we should open a new bug to convert all the uses of “g_object_{g,s}et_data” to use the “qdata” variants, WDYT? > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:477 > +} The “didSniffContent()” member function is private and only ever called from the “didSniffContentCallback()” function right above. I think it's may not be worth it to have this function containing a single assignment, it could be done just directly inside “didSniffContentCallback()” — though I have no strong preference, feel free to leave this as-is.
Carlos Garcia Campos
Comment 3 2021-01-26 03:53:38 PST
Comment on attachment 417962 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417962&action=review >> Source/WebCore/platform/network/soup/ResourceRequest.h:85 >> + bool m_acceptEncoding : 1; > > It does not make sense to specify a bit width of “1” here. This is the > only member for which the bit width is indicated, which will result in > the compiler adding padding anyway—which defeats the intent of saving > some bytes of memory. We may as well do: > > bool m_acceptEncoding { true }; > > …and skip setting the initial value in the constructors above. > > Just tested: > > % cat > t.cc <<EOF > #include <cstdio> > enum Flag { FLAG_FOO, FLAG_BAR, FLAG_BAZ }; > class X { bool bit: 1; enum Flag f; }; > class Y { bool bit; enum Flag f; }; > int main() { > std::printf("X: %zu bytes, Y: %zu bytes\n", sizeof(X), sizeof(Y)); > } > EOF > % g++ -o t t.cc && ./t > X: 8 bytes, Y: 8 bytes > % Sure. >> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:333 >> + g_object_set_data_full(G_OBJECT(task->m_pendingResult.get()), "wk-send-request-data", protectedData.release(), [](gpointer data) { > > Is there any reason not to use “g_object_set_qdata_full()”? That would > avoid the string→GQuark conversion on each usage. Maybe we should open > a new bug to convert all the uses of “g_object_{g,s}et_data” to use the > “qdata” variants, WDYT? I don't know if it really has any impact. But it's probably better to do that in a separate commit for all the set_data cases instead. >> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:477 >> +} > > The “didSniffContent()” member function is private and only ever called from > the “didSniffContentCallback()” function right above. I think it's may not be > worth it to have this function containing a single assignment, it could be done > just directly inside “didSniffContentCallback()” — though I have no strong > preference, feel free to leave this as-is. Yes, this is just to follow the pattern for all other async ops in this file, didFooCallback -> didFoo
Carlos Garcia Campos
Comment 4 2021-01-26 03:55:24 PST
Created attachment 418390 [details] Patch for landing
Carlos Garcia Campos
Comment 5 2021-01-26 04:33:21 PST
Created attachment 418394 [details] Patch for landing
EWS Watchlist
Comment 6 2021-01-26 04:47:07 PST
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Lauro Moura
Comment 7 2021-01-26 20:44:18 PST
Linking the bug with the regressions from r271879. (Should have gardened with this one instead).
Carlos Garcia Campos
Comment 8 2021-01-28 04:42:22 PST
Note You need to log in before you can comment on or make changes to this bug.