[webkit-reviews] review granted: [Bug 220764] [SOUP] Stop using SoupRequest API in preparation for libsoup3 : [Attachment 417962] Patch

[bugzilla-daemon at webkit.org]

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

Attachment 417962: Patch

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




--- Comment #2 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 417962
  --> https://bugs.webkit.org/attachment.cgi?id=417962
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.