[webkit-reviews] review granted: [Bug 186312] Network Preflights do not show in WebInspector after moving CORS checks to NetworkProcess : [Attachment 342691] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 186312: Network Preflights do not show in WebInspector after moving CORS
checks to NetworkProcess
https://bugs.webkit.org/show_bug.cgi?id=186312

Attachment 342691: Patch

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




--- Comment #13 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 342691
  --> https://bugs.webkit.org/attachment.cgi?id=342691
Patch

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

r=me with changes.

> Source/WebCore/testing/Internals.cpp:4577
> +String Internals::ongoingLoadDescriptions()

Shouldn't this be named ongoingLoadsDescriptions() ?

> Source/WebCore/testing/Internals.cpp:4580
> +    builder.append("[");

'[' is more efficient than "[".

> Source/WebCore/testing/Internals.cpp:4581
> +    bool isStarting = true;

I think isFirst may be a bit clearer.

> Source/WebCore/testing/Internals.cpp:4582
> +    auto loads = platformStrategies()->loaderStrategy()->ongoingLoads();

This variable is unused, please drop this line. The loop below is fine.

> Source/WebCore/testing/Internals.cpp:4587
> +	       builder.append(",");

',' not ","

> Source/WebCore/testing/Internals.cpp:4589
> +	   builder.append("[");

ditto.

> Source/WebCore/testing/Internals.cpp:4592
> +	       builder.append(makeString("[", (int)info.type, ",\"",
info.request.url().string(), "\",\"", info.request.httpMethod(), "\",",
info.response.httpStatusCode(), "]"));

Is is really worth calling makeString() just to append to a StringBuilder? I
would just append every part individually to the StringBuilder.

> Source/WebCore/testing/Internals.cpp:4594
> +	   builder.append("]");

']'

> Source/WebCore/testing/Internals.cpp:4596
> +    builder.append("]");

']'

> Source/WebCore/testing/Internals.h:700
> +    String ongoingLoadDescriptions();

const ?

> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.h:77
> +    WebCore::NetworkTransactionInformation m_loadInformation;

I personally think this could be an std::optional given this is only set when
m_shouldCaptureExtraNetworkLoadMetrics is true.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:81
> +    void takeNetworkLoadInformationRequest(ResourceLoadIdentifier
identifier, WebCore::ResourceRequest& request)

This is not really "taking".

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:91
> +    void takeNetworkLoadIntermediateInformation(ResourceLoadIdentifier
identifier, Vector<WebCore::NetworkTransactionInformation>& information)

This is not really "taking".

> Source/WebKit/NetworkProcess/PingLoad.cpp:45
> +    , m_networkLoadChecker(makeUniqueRef<NetworkLoadChecker>(connection,
m_parameters.webPageID, m_parameters.webFrameID, m_parameters.identifier,
FetchOptions { m_parameters.options}, m_parameters.sessionID,
WTFMove(m_parameters.originalRequestHeaders), URL { m_parameters.request.url()
}, m_parameters.sourceOrigin.copyRef(), m_parameters.preflightPolicy,
m_parameters.request.httpReferrer(), false))

false is not nice here. I think we should use an enum class instead of a
boolean.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.h:103
> +	   return WTF::map(m_webResourceLoaders, [](auto&& keyValue) ->
uint64_t {

why auto&& and not auto& ?
Do we really need the "-> uint64_t" ?

> LayoutTests/http/wpt/fetch/resources/preflight.py:16
> +    time.sleep(5);

That's not 5 seconds, is it?