Bug 196807

Created attachment 367198 [details] Patch

Attachment 367198 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoadMap.h:58: 'take' is incorrectly named. It should be named 'protector' or 'protectedResourceLoadIdentifier'. [readability/naming/protected] [4] ERROR: Source/WebCore/loader/LoaderStrategy.h:70: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 2 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 367198 [details] Patch Attachment 367198 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/11837791 New failing tests: http/tests/security/contentSecurityPolicy/report-blocked-uri-and-do-not-follow-redirect-when-sending-report.php http/tests/security/xssAuditor/report-script-tag-full-block-and-do-not-follow-redirect-when-sending-report.html http/tests/security/xssAuditor/report-script-tag-and-do-not-follow-redirect-when-sending-report.html http/tests/security/contentSecurityPolicy/user-style-sheet-font-crasher.php

Created attachment 367201 [details] Archive of layout-test-results from ews101 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6

Created attachment 367255 [details] Patch

Attachment 367255 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoadMap.h:58: 'take' is incorrectly named. It should be named 'protector' or 'protectedResourceLoadIdentifier'. [readability/naming/protected] [4] ERROR: Source/WebCore/loader/LoaderStrategy.h:70: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 2 in 43 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 367328 [details] Patch

Attachment 367328 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoadMap.h:58: 'take' is incorrectly named. It should be named 'protector' or 'protectedResourceLoadIdentifier'. [readability/naming/protected] [4] ERROR: Source/WebCore/loader/LoaderStrategy.h:70: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 2 in 42 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 367334 [details] Patch

Attachment 367334 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoadMap.h:58: 'take' is incorrectly named. It should be named 'protector' or 'protectedResourceLoadIdentifier'. [readability/naming/protected] [4] ERROR: Source/WebCore/loader/LoaderStrategy.h:70: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 2 in 42 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 367334 [details] Patch Attachment 367334 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/11856074 New failing tests: http/tests/security/contentSecurityPolicy/user-style-sheet-font-crasher.php

Created attachment 367339 [details] Archive of layout-test-results from ews102 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-highsierra Platform: Mac OS X 10.13.6

Comment on attachment 367334 [details] Patch Attachment 367334 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11856089 New failing tests: http/tests/ssl/referer-301.html

Created attachment 367340 [details] Archive of layout-test-results from ews104 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6

Comment on attachment 367334 [details] Patch Attachment 367334 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11856333 New failing tests: http/tests/security/contentSecurityPolicy/user-style-sheet-font-crasher.php

Created attachment 367342 [details] Archive of layout-test-results from ews115 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-highsierra Platform: Mac OS X 10.13.6

Created attachment 367352 [details] Patch

Attachment 367352 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoadMap.h:59: 'take' is incorrectly named. It should be named 'protector' or 'protectedResourceLoadIdentifier'. [readability/naming/protected] [4] ERROR: Source/WebCore/loader/LoaderStrategy.h:70: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 2 in 42 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 367359 [details] Patch

Attachment 367359 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoadMap.h:59: 'take' is incorrectly named. It should be named 'protector' or 'protectedResourceLoadIdentifier'. [readability/naming/protected] [4] ERROR: Source/WebCore/loader/LoaderStrategy.h:70: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 2 in 38 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 368138 [details] Patch

Attachment 368138 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoadMap.h:59: 'take' is incorrectly named. It should be named 'protector' or 'protectedResourceLoadIdentifier'. [readability/naming/protected] [4] ERROR: Source/WebCore/loader/LoaderStrategy.h:70: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 2 in 40 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 368138 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368138&action=review I like this. I would like it even more if we could completely remove PingLoad, but I guess that's coming. This makes our loading code more like the current specification. > Source/WebCore/loader/cache/CachedResourceLoader.cpp:775 > case CachedResource::Type::Beacon: > + case CachedResource::Type::Ping: Do we want these to be distinct types? Will we add another type for fetch keep-alive? > Source/WebKit/NetworkProcess/NetworkSession.h:109 > + HashSet<Ref<NetworkResourceLoader>> m_keptAliveLoads; I think it would be better if this were on the NetworkSession. That would make cleanup a lot more intuitive when we remove a private browsing session, at which point I assume we would want to cancel all the outstanding pings, beacons, and keep-alive fetches, right? > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:577 > + return !RuntimeEnabledFeatures::sharedFeatures().fetchAPIKeepAliveEnabled(); Won't we soon never want to use ping load in modern WebKit?

> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:775 > > case CachedResource::Type::Beacon: > > + case CachedResource::Type::Ping: > > Do we want these to be distinct types? Will we add another type for fetch > keep-alive? Fetch keep-alive will be tied to regular fetch. Ping are tied to some specific loads that are worth marking as such. > > Source/WebKit/NetworkProcess/NetworkSession.h:109 > > + HashSet<Ref<NetworkResourceLoader>> m_keptAliveLoads; > > I think it would be better if this were on the NetworkSession. That would > make cleanup a lot more intuitive when we remove a private browsing session, > at which point I assume we would want to cancel all the outstanding pings, > beacons, and keep-alive fetches, right? m_keptAliveLoads is in the NetworkSession. I am not sure what you are suggesting here. > > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:577 > > + return !RuntimeEnabledFeatures::sharedFeatures().fetchAPIKeepAliveEnabled(); > > Won't we soon never want to use ping load in modern WebKit? Definitely, I hope to enable it soon after it runs a few cycle on our bots.

(In reply to Alex Christensen from comment #23) > Comment on attachment 368138 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=368138&action=review > > I like this. I would like it even more if we could completely remove > PingLoad, but I guess that's coming. This makes our loading code more like > the current specification. You r-ed it but I do not see any objection or suggestion for improvement. Did you hit the wrong button?

Comment on attachment 368138 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368138&action=review r=me > Source/WebKit/ChangeLog:11 > + and add it to a kept-alive NetworkProcess load set. The loader is only kept alive if it Oh, I still had this in my mind. I guess this needs to be updated.

Created attachment 368329 [details] Patch for landing

*** Bug 194392 has been marked as a duplicate of this bug. ***

Attachment 368329 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoadMap.h:59: 'take' is incorrectly named. It should be named 'protector' or 'protectedResourceLoadIdentifier'. [readability/naming/protected] [4] ERROR: Source/WebCore/loader/LoaderStrategy.h:70: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 2 in 40 files If any of these errors are false positives, please file a bug against check-webkit-style.

<rdar://problem/50247053>

<rdar://problem/50247059>

The commit-queue encountered the following flaky tests while processing attachment 368329 [details]: fetch/fetch-worker-crash.html bug 187257 (author: youennf@gmail.com) The commit-queue is continuing to process your patch.

Comment on attachment 368329 [details] Patch for landing Clearing flags on attachment: 368329 Committed r244700: <https://trac.webkit.org/changeset/244700>

All reviewed patches have been landed. Closing bug.

Comment on attachment 368329 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=368329&action=review > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:488 > + if (m_isKeptAlive) { > + m_responseCompletionHandler = WTFMove(completionHandler); > + RunLoop::main().dispatch([protectedThis = makeRef(*this)] { > + protectedThis->didFinishLoading(NetworkLoadMetrics { }); > + }); > + return; > + } I'm simplifying this in https://bugs.webkit.org/show_bug.cgi?id=204134