Bug 220764 – [SOUP] Stop using SoupRequest API in preparation for libsoup3

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

It's gone in libsoup3, we can just use soup_session_send_async() instead, which is what SoupRequest does internally.

[Carlos Garcia Campos]

Created attachment 417962 [details]
Patch

[Adrian Perez]

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 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]

Created attachment 418390 [details]
Patch for landing

[Carlos Garcia Campos]

Created attachment 418394 [details]
Patch for landing

[EWS Watchlist]

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]

Linking the bug with the regressions from r271879. (Should have gardened with this one instead).

[Carlos Garcia Campos]

Committed r272010: <https://trac.webkit.org/changeset/272010>