Network Preflights do not show in WebInspector after moving CORS checks to NetworkProcess
<rdar://problem/40495434>
Created attachment 341982 [details] Patch
Comment on attachment 341982 [details] Patch Attachment 341982 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/8013907 New failing tests: http/tests/inspector/network/har/har-page.html http/tests/inspector/network/fetch-network-data.html
Created attachment 341985 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 341994 [details] Patch
Created attachment 342616 [details] Patch
Comment on attachment 342616 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342616&action=review LGTM, but I'd like someone on your team to also take a peek at NetworkProcess changes. > Source/WebCore/platform/network/NetworkLoadInformation.h:36 > +struct NetworkIntermediateLoadInformation { 'Intermediate' seems weird here, what about Provisional? How are redirects called in the rest of loading code? > Source/WebCore/platform/network/NetworkLoadInformation.h:70 > + encoder << type << request << response << metrics; I believe we use separate lines to make this easier to diff in the future. > LayoutTests/ChangeLog:15 > +2018-06-12 Youenn Fablet <youenn@apple.com> Nit: double changelog
Thanks for the review. (In reply to Brian Burg from comment #7) > Comment on attachment 342616 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=342616&action=review > > LGTM, but I'd like someone on your team to also take a peek at > NetworkProcess changes. > > > Source/WebCore/platform/network/NetworkLoadInformation.h:36 > > +struct NetworkIntermediateLoadInformation { > > 'Intermediate' seems weird here, what about Provisional? How are redirects > called in the rest of loading code? I don't know if there is a name coined for a single request/response exchange. Maybe NetworkTransactionInformation would be more appropriate. > > Source/WebCore/platform/network/NetworkLoadInformation.h:70 > > + encoder << type << request << response << metrics; > > I believe we use separate lines to make this easier to diff in the future. OK > > LayoutTests/ChangeLog:15 > > +2018-06-12 Youenn Fablet <youenn@apple.com> > > Nit: double changelog OK
Created attachment 342676 [details] Patch
Comment on attachment 342676 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342676&action=review > Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:445 > +void NetworkLoadChecker::storeRedirectionIfNeeded(WebCore::ResourceRequest&& request, WebCore::ResourceResponse&& response) It would improve performance to make this function take const references and only copy if needed.
Created attachment 342691 [details] Patch
(In reply to Alex Christensen from comment #10) > Comment on attachment 342676 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=342676&action=review > > > Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:445 > > +void NetworkLoadChecker::storeRedirectionIfNeeded(WebCore::ResourceRequest&& request, WebCore::ResourceResponse&& response) > > It would improve performance to make this function take const references and > only copy if needed. Right, done!
Comment on attachment 342691 [details] 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?
Created attachment 343052 [details] Patch for landing
Comment on attachment 343052 [details] Patch for landing Clearing flags on attachment: 343052 Committed r232963: <https://trac.webkit.org/changeset/232963>
All reviewed patches have been landed. Closing bug.
(In reply to Chris Dumez from comment #13) > Comment on attachment 342691 [details] > Patch Thanks, I updated accordingly. > > LayoutTests/http/wpt/fetch/resources/preflight.py:16 > > + time.sleep(5); > > That's not 5 seconds, is it? It is but the test is not waiting for the fetch load to terminate.