TestController should not exercise cocoa-specific resource load statistics APIs. I don't know if this is possible yet; let's see what EWS thinks.
Created attachment 332808 [details] Patch
Created attachment 332817 [details] Patch
If EWS passes, then the next question is whether the SPI Source/WebKit/UIProcess/API/Cocoa should be removed? It's not clear to me whether those are internal or public.
Created attachment 332820 [details] Patch
Some rationale: The TestController is cross-platform code, so it doesn't make sense to use Objective C++ here except when there is actual platform-specific work to do. If I can get EWS green, then all the tests pass on Mac when I remove the Cocoa implementation and switch it to the cross-platform implementation. Then surely we don't need to keep the Cocoa implementation that works exactly the same as the cross-platform implementation. I might be wrong, but I think the real goal is to get rid of the WK2 C API, right? I believe no ports need or want it now, *except* that it's still needed for the TestController. (The C++ API seems to be needed in a couple more places, but hopefully that can eventually go away too.) If we want to make progress on getting rid of the C API, we should try to come up with a plan that allows us to achieve that without resulting in three separate Windows, Mac, and Linux TestControllers: I assume nobody wants that. One solution would be to add e.g. a WEBKIT_EXPORT_PRIVATE macro, to export arbitrary APIs needed by TestController in developer builds, so that TestController can depend on WebKit internals. I'm sure there are other options.
Created attachment 332836 [details] Patch
Created attachment 332837 [details] Patch
Weird, the Mac EWS is happy, but iOS has a bunch of failures. Needs further investigation. Still, the proof of concept is there. Alex, John, what do you think...?
The next step would be to remove use of the WK2 C API. Strawman proposal would be to take code that looks like this: void TestController::statisticsClearThroughWebsiteDataRemoval() { auto* dataStore = WKContextGetWebsiteDataStore(platformContext()); WKWebsiteDataStoreStatisticsClearThroughWebsiteDataRemoval(dataStore, this, statisticsClearThroughWebsiteDataRemovalCallback); } And just replace it with the C API impl, something like: void TestController::statisticsClearThroughWebsiteDataRemoval() { WebsiteDataStore* dataStore = somehowGetPlatformProcessPool().websiteDataStore(); OptionSet<WebKit::WebsiteDataType> dataTypes = WebKit::WebsiteDataType::ResourceLoadStatistics; dataStore->removeData(dataTypes, WallTime::fromRawSeconds(0), statisticsClearThroughWebsiteDataRemovalCallback); } There would be details to work out, e.g. WebProcessPool::websiteDataStore actually currently returns an API::WebsiteDataStore, which we probably want to get rid of, and we would have to introduce a WEBKIT_EXPORT_PRIVATE to export these symbols for the TestController to use in developer builds. But on the whole, it seems like it should be straightforward, and a realistic path to getting rid of the WK2 C and C++ APIs (which is desired, right?).
(In reply to Michael Catanzaro from comment #8) > Weird, the Mac EWS is happy, but iOS has a bunch of failures. Needs further > investigation. > > Still, the proof of concept is there. Alex, John, what do you think...? This is probably because Mac EWS runs on an older version of macOS where Resource Load Statistics are not supported and thus tests are skipped. You’ll see in platform test expectations that most tests are marked HighSierra+. iOS however, is current, so that’s where you’ll see test breakage.
Comment on attachment 332837 [details] Patch Attachment 332837 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/6311760 New failing tests: http/tests/resourceLoadStatistics/third-party-cookie-with-and-without-user-interaction.html http/tests/resourceLoadStatistics/partitioned-cookies-with-and-without-user-interaction.html http/tests/resourceLoadStatistics/add-partitioning-to-redirect.html http/tests/resourceLoadStatistics/remove-blocking-in-redirect.html http/tests/resourceLoadStatistics/non-prevalent-resources-can-access-cookies-in-a-third-party-context.html http/tests/resourceLoadStatistics/remove-partitioning-in-redirect.html http/tests/resourceLoadStatistics/add-blocking-to-redirect.html http/tests/resourceLoadStatistics/partitioned-and-unpartitioned-cookie-with-partitioning-timeout.html http/tests/resourceLoadStatistics/partitioned-and-unpartitioned-cookie-deletion.html
Created attachment 332850 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 332837 [details] Patch Attachment 332837 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/6312803 New failing tests: http/tests/resourceLoadStatistics/third-party-cookie-with-and-without-user-interaction.html http/tests/resourceLoadStatistics/partitioned-cookies-with-and-without-user-interaction.html http/tests/resourceLoadStatistics/add-partitioning-to-redirect.html http/tests/resourceLoadStatistics/remove-blocking-in-redirect.html http/tests/resourceLoadStatistics/non-prevalent-resources-can-access-cookies-in-a-third-party-context.html http/tests/resourceLoadStatistics/remove-partitioning-in-redirect.html http/tests/resourceLoadStatistics/add-blocking-to-redirect.html http/tests/resourceLoadStatistics/partitioned-and-unpartitioned-cookie-with-partitioning-timeout.html http/tests/resourceLoadStatistics/partitioned-and-unpartitioned-cookie-deletion.html
Created attachment 332856 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
(In reply to John Wilander from comment #10) > This is probably because Mac EWS runs on an older version of macOS where > Resource Load Statistics are not supported and thus tests are skipped. > You’ll see in platform test expectations that most tests are marked > HighSierra+. iOS however, is current, so that’s where you’ll see test > breakage. Hmmm OK, I'll need to figure out what's missing from the C API to proceed with this, then. But, from bug #181958, I suspect Alex won't want to do this anyway. In that case, it would be helpful to suggest some sort of alternative: what would Apple like to see from WKTR, and how can Igalia help to achieve that goal?
I think this patch will be a step in a good direction.
Created attachment 333021 [details] Patch
Let's see if EWS likes this next attempt any better.... (In reply to Alex Christensen from comment #16) > I think this patch will be a step in a good direction. Now I'm really confused as to what you are looking for, since in bug #181958 comment 22 you indicated that I should not be doing exactly this. But anyway, here's another attempt. Please let me know what you are looking for here! If you want me to go the next step and try ripping out the C API use, I could start experimenting with that that too... just let me know what you want.
Created attachment 333022 [details] Patch
Created attachment 333023 [details] Patch
Comment on attachment 333023 [details] Patch Green! :)
(In reply to Alex Christensen from comment #16) > I think this patch will be a step in a good direction. r?
Comment on attachment 333023 [details] Patch Rejecting attachment 333023 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 333023, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: succeeded at 2723 with fuzz 1 (offset 5 lines). patching file Tools/WebKitTestRunner/TestController.h Hunk #1 succeeded at 338 (offset 2 lines). patching file Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm Hunk #2 FAILED at 249. 1 out of 2 hunks FAILED -- saving rejects to file Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm.rej Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Alex Christensen']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.webkit.org/results/6420288
In bug #181958 comment 22 I was indicating that the initial work was done on cocoa only and because this hadn't been done we were continuing in that direction for historical reasons. This seems good. I'm pretty sure removing all this SPI won't cause any problem because the test app is the only thing that was using them.
OK, sounds good.
Created attachment 333424 [details] Patch
Created attachment 333428 [details] Patch
Created attachment 333433 [details] Patch
Committed r228304: <https://trac.webkit.org/changeset/228304>