Summary: | [SOUP] Stop using SoupRequest API in preparation for libsoup3 | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aperez, berto, bugs-noreply, ews-watchlist, gustavo, lmoura | ||||||||
Priority: | P2 | Keywords: | Gtk | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=221023 | ||||||||||
Bug Depends on: | 220509, 220981 | ||||||||||
Bug Blocks: | 220508, 221034 | ||||||||||
Attachments: |
|
Description
Carlos Garcia Campos
2021-01-20 05:18:33 PST
Created attachment 417962 [details]
Patch
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. 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 Created attachment 418390 [details]
Patch for landing
Created attachment 418394 [details]
Patch for landing
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 Linking the bug with the regressions from r271879. (Should have gardened with this one instead). Committed r272010: <https://trac.webkit.org/changeset/272010> |