rdar://problem/60943970
Created attachment 394683 [details] Patch
Comment on attachment 394683 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394683&action=review > Source/WebKit/ChangeLog:10 > + Add the ability to configure a website data store so that its network session operates > + using WebCore::FirstPartyWebsiteDataRemovalMode::None. This needs tests.
Comment on attachment 394683 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394683&action=review > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreConfiguration.h:203 > + bool m_resourceLoadStatisticsFirstPartyWebsiteDataRemovalEnabled { true }; This should not be inside PLATFORM(COCOA)
Created attachment 395284 [details] Patch
Comment on attachment 395284 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395284&action=review > Source/WebKit/UIProcess/API/Cocoa/_WKWebsiteDataStoreConfiguration.h:77 > +@property (nonatomic, nullable, copy, setter=setStandaloneApplication:) NSURL *standaloneApplicationURL WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA)); Why explicitly specify the setter -setStandaloneApplication: and not just use the default -setStandaloneApplicationURL: ?
(In reply to David Quesada from comment #5) > Comment on attachment 395284 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=395284&action=review > > > Source/WebKit/UIProcess/API/Cocoa/_WKWebsiteDataStoreConfiguration.h:77 > > +@property (nonatomic, nullable, copy, setter=setStandaloneApplication:) NSURL *standaloneApplicationURL WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA)); > > Why explicitly specify the setter -setStandaloneApplication: and not just > use the default -setStandaloneApplicationURL: ? I got build errors without the explicit setter but Alex made some suggestions so I'll remove it before landing.
Comment on attachment 395284 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395284&action=review > Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:156 > + if (options.useEphemeralSession && options.standaloneWebApplicationURL.length()) { This could be made a bit more concise and expandable: if (options.useEphemeralSession || options.standaloneWebApplicationURL.length()) { auto config = options.useEphemeralSession ? [[[_WKWebsiteDataStoreConfiguration alloc] initNonPersistentConfiguration] autorelease] : [[[_WKWebsiteDataStoreConfiguration alloc] init] autorelease]; if (options.standaloneWebApplicationURL.length()) config.standaloneApplication = ...; // we will probably want to add more things here soon. ... // set websiteDataStore using config and enable resource load statistics. } > Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:157 > + auto ephemeralWebsiteDataStoreConfig = [[_WKWebsiteDataStoreConfiguration alloc] initNonPersistentConfiguration]; This is a memory leak as it now stands.
(In reply to Alex Christensen from comment #7) > Comment on attachment 395284 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=395284&action=review > > > Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:156 > > + if (options.useEphemeralSession && options.standaloneWebApplicationURL.length()) { > > This could be made a bit more concise and expandable: > > if (options.useEphemeralSession > || options.standaloneWebApplicationURL.length()) { > auto config = options.useEphemeralSession ? > [[[_WKWebsiteDataStoreConfiguration alloc] initNonPersistentConfiguration] > autorelease] : [[[_WKWebsiteDataStoreConfiguration alloc] init] autorelease]; > if (options.standaloneWebApplicationURL.length()) > config.standaloneApplication = ...; > // we will probably want to add more things here soon. > ... // set websiteDataStore using config and enable resource load > statistics. > } Much better. Will adopt. > > Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:157 > > + auto ephemeralWebsiteDataStoreConfig = [[_WKWebsiteDataStoreConfiguration alloc] initNonPersistentConfiguration]; > > This is a memory leak as it now stands. Darn it. I thought I had gone through all of them. Will fix.
Created attachment 395321 [details] Patch
Comment on attachment 395321 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395321&action=review > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebsiteDatastore.mm:312 > +TEST(WKWebsiteDataStore, StandaloneApplicationURL) This test isn't really useful. It just verifies that properties are copied correctly.
(In reply to Alex Christensen from comment #10) > Comment on attachment 395321 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=395321&action=review > > > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebsiteDatastore.mm:312 > > +TEST(WKWebsiteDataStore, StandaloneApplicationURL) > > This test isn't really useful. It just verifies that properties are copied > correctly. Right, it was added to make sure the setting is maintained through copies of the config. Do you think I should remove it?
yes
(In reply to Alex Christensen from comment #12) > yes Will do before landing. Waiting for the wk2 bots now. Thanks for the review!
Created attachment 395332 [details] Patch for landing
Committed r259440: <https://trac.webkit.org/changeset/259440> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395332 [details].
It looks like these two new tests: http/tests/resourceLoadStatistics/standalone-web-application-exempt-from-website-data-deletion-database.html http/tests/resourceLoadStatistics/standalone-web-application-exempt-from-website-data-deletion.html added in https://trac.webkit.org/changeset/259440/webkit are failing or timing out. History: https://results.webkit.org/?suite=layout-tests&suite=layout-tests&test=http%2Ftests%2FresourceLoadStatistics%2Fstandalone-web-application-exempt-from-website-data-deletion.html&test=http%2Ftests%2FresourceLoadStatistics%2Fstandalone-web-application-exempt-from-website-data-deletion-database.html Results: https://build.webkit.org/results/Apple-Catalina-Release-WK2-Tests/r259460%20(4334)/results.html
(In reply to Truitt Savell from comment #16) > It looks like these two new tests: > > http/tests/resourceLoadStatistics/standalone-web-application-exempt-from- > website-data-deletion-database.html > http/tests/resourceLoadStatistics/standalone-web-application-exempt-from- > website-data-deletion.html > > added in https://trac.webkit.org/changeset/259440/webkit > > are failing or timing out. History: > https://results.webkit.org/?suite=layout-tests&suite=layout- > tests&test=http%2Ftests%2FresourceLoadStatistics%2Fstandalone-web- > application-exempt-from-website-data-deletion. > html&test=http%2Ftests%2FresourceLoadStatistics%2Fstandalone-web-application- > exempt-from-website-data-deletion-database.html > > Results: > https://build.webkit.org/results/Apple-Catalina-Release-WK2-Tests/ > r259460%20(4334)/results.html Thanks for reporting, Truitt. Will look into this.
(In reply to John Wilander from comment #17) > (In reply to Truitt Savell from comment #16) > > It looks like these two new tests: > > > > http/tests/resourceLoadStatistics/standalone-web-application-exempt-from- > > website-data-deletion-database.html > > http/tests/resourceLoadStatistics/standalone-web-application-exempt-from- > > website-data-deletion.html > > > > added in https://trac.webkit.org/changeset/259440/webkit > > > > are failing or timing out. History: > > https://results.webkit.org/?suite=layout-tests&suite=layout- > > tests&test=http%2Ftests%2FresourceLoadStatistics%2Fstandalone-web- > > application-exempt-from-website-data-deletion. > > html&test=http%2Ftests%2FresourceLoadStatistics%2Fstandalone-web-application- > > exempt-from-website-data-deletion-database.html > > > > Results: > > https://build.webkit.org/results/Apple-Catalina-Release-WK2-Tests/ > > r259460%20(4334)/results.html > > Thanks for reporting, Truitt. Will look into this. The linked failure seems to be one missing line in the output, namely the HttpOnly cookie. The weird thing is that it's not saying the cookie has been deleted. There just isn't a result for that cookie. Will try to reproduce locally and do a manual audit of the code.
(In reply to John Wilander from comment #18) > (In reply to John Wilander from comment #17) > > (In reply to Truitt Savell from comment #16) > > > It looks like these two new tests: > > > > > > http/tests/resourceLoadStatistics/standalone-web-application-exempt-from- > > > website-data-deletion-database.html > > > http/tests/resourceLoadStatistics/standalone-web-application-exempt-from- > > > website-data-deletion.html > > > > > > added in https://trac.webkit.org/changeset/259440/webkit > > > > > > are failing or timing out. History: > > > https://results.webkit.org/?suite=layout-tests&suite=layout- > > > tests&test=http%2Ftests%2FresourceLoadStatistics%2Fstandalone-web- > > > application-exempt-from-website-data-deletion. > > > html&test=http%2Ftests%2FresourceLoadStatistics%2Fstandalone-web-application- > > > exempt-from-website-data-deletion-database.html > > > > > > Results: > > > https://build.webkit.org/results/Apple-Catalina-Release-WK2-Tests/ > > > r259460%20(4334)/results.html > > > > Thanks for reporting, Truitt. Will look into this. > > The linked failure seems to be one missing line in the output, namely the > HttpOnly cookie. The weird thing is that it's not saying the cookie has been > deleted. There just isn't a result for that cookie. > > Will try to reproduce locally and do a manual audit of the code. I've now run 50 iterations with a release build without a repro of the missing output line.
Reverted r259440 for reason: Introduced 2 failing tests on Mac and iOS Committed r259516: <https://trac.webkit.org/changeset/259516>
Created attachment 396595 [details] Patch
Comment on attachment 396595 [details] Patch mac-debug-wk1 failures are unrelated.
Committed r260169: <https://trac.webkit.org/changeset/260169> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396595 [details].
<rdar://problem/61891463> tracks the re-landing.