Bug 182355 – TestController should not exercise cocoa-specific resource load statistics APIs

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 332808 [details]
Patch

[Michael Catanzaro]

Created attachment 332817 [details]
Patch

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 332820 [details]
Patch

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 332836 [details]
Patch

[Michael Catanzaro]

Created attachment 332837 [details]
Patch

[Michael Catanzaro]

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...?

[Michael Catanzaro]

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?).

[John Wilander]

(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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Michael Catanzaro]

(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?

[Alex Christensen]

I think this patch will be a step in a good direction.

[Michael Catanzaro]

Created attachment 333021 [details]
Patch

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 333022 [details]
Patch

[Michael Catanzaro]

Created attachment 333023 [details]
Patch

[Michael Catanzaro]

Comment on attachment 333023 [details]
Patch

Green! :)

[Michael Catanzaro]

(In reply to Alex Christensen from comment #16)
> I think this patch will be a step in a good direction.

r?

[WebKit Commit Bot]

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

[Alex Christensen]

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.

[Michael Catanzaro]

OK, sounds good.

[Michael Catanzaro]

Created attachment 333424 [details]
Patch

[Michael Catanzaro]

Created attachment 333428 [details]
Patch

[Michael Catanzaro]

Created attachment 333433 [details]
Patch

[Michael Catanzaro]

Committed r228304: <https://trac.webkit.org/changeset/228304>