[webkit-reviews] review granted: [Bug 207747] NetworkLoadMetrics should be shared by multiple ResourceResponse instances : [Attachment 390755] Patch

[bugzilla-daemon at webkit.org]

Keith Miller <keith_miller at apple.com> has granted Yusuke Suzuki
<ysuzuki at apple.com>'s request for review:
Bug 207747: NetworkLoadMetrics should be shared by multiple ResourceResponse
instances
https://bugs.webkit.org/show_bug.cgi?id=207747

Attachment 390755: Patch

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




--- Comment #12 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 390755
  --> https://bugs.webkit.org/attachment.cgi?id=390755
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390755&action=review

r=me with nits.

> Source/WebCore/ChangeLog:13
> +	   While we sometimes copies it to modify some part of it,
NetworkLoadMetrics is immutable. It is set when response is created,

nit: copies => copy

Also, what is "it" in "copies it"? A NeworkLoadMetrics or a ResourceResponse?

> Source/WebCore/ChangeLog:17
> +	   This patch puts Box<NetworkLoadMetrics> in ResourceResponse to share
it by all copied ResourceResponses. We do not make NetworkLoadMetrics

Nit: share it by => hare it with.

> Source/WebCore/ChangeLog:18
> +	   RefCounted<> for now since some legit data structure embeds
NetworkLoadMetrics. This patch adds ArgumentCoder for Box so that we

Nit: data structure embeds => data structures embed

Or did you mean that there is only on data structure that embed's
NetworkLoadMetrics? If so can you name that class?

> Source/WebCore/platform/network/ResourceResponseBase.h:339
> +    if constexpr (Decoder::isIPCDecoder) {
> +	   if (!decoder.decode(response.m_networkLoadMetrics))
> +	       return false;
> +    }

Why bother with this change? Seems like the compiler will make this
optimization anyway?

> Source/WebCore/platform/network/cocoa/NetworkLoadMetrics.mm:68
> +

Nit: remove newline here.