Bug 182355 - TestController should not exercise cocoa-specific resource load statistics APIs
Summary: TestController should not exercise cocoa-specific resource load statistics APIs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-01-31 15:19 PST by Michael Catanzaro
Modified: 2018-02-09 11:20 PST (History)
9 users (show)

See Also:


Attachments
Patch (12.82 KB, patch)
2018-01-31 15:20 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (12.75 KB, patch)
2018-01-31 16:12 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (13.20 KB, patch)
2018-01-31 16:30 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (13.41 KB, patch)
2018-01-31 18:51 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (16.24 KB, patch)
2018-01-31 18:58 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (57.65 MB, application/zip)
2018-01-31 23:25 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (54.38 MB, application/zip)
2018-02-01 01:25 PST, EWS Watchlist
no flags Details
Patch (50.09 KB, patch)
2018-02-02 17:30 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (50.06 KB, patch)
2018-02-02 17:40 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (49.44 KB, patch)
2018-02-02 17:51 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (49.91 KB, patch)
2018-02-08 14:34 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (49.75 KB, patch)
2018-02-08 15:18 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (50.06 KB, patch)
2018-02-08 16:14 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2018-01-31 15:19:10 PST
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.
Comment 1 Michael Catanzaro 2018-01-31 15:20:27 PST
Created attachment 332808 [details]
Patch
Comment 2 Michael Catanzaro 2018-01-31 16:12:08 PST
Created attachment 332817 [details]
Patch
Comment 3 Michael Catanzaro 2018-01-31 16:13:26 PST
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.
Comment 4 Michael Catanzaro 2018-01-31 16:30:58 PST
Created attachment 332820 [details]
Patch
Comment 5 Michael Catanzaro 2018-01-31 18:21:05 PST
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.
Comment 6 Michael Catanzaro 2018-01-31 18:51:42 PST
Created attachment 332836 [details]
Patch
Comment 7 Michael Catanzaro 2018-01-31 18:58:35 PST
Created attachment 332837 [details]
Patch
Comment 8 Michael Catanzaro 2018-01-31 20:20:26 PST
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...?
Comment 9 Michael Catanzaro 2018-01-31 21:07:28 PST
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?).
Comment 10 John Wilander 2018-01-31 21:19:58 PST
(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 11 EWS Watchlist 2018-01-31 23:25:47 PST
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
Comment 12 EWS Watchlist 2018-01-31 23:25:49 PST
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 13 EWS Watchlist 2018-02-01 01:25:42 PST
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
Comment 14 EWS Watchlist 2018-02-01 01:25:51 PST
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
Comment 15 Michael Catanzaro 2018-02-01 06:43:56 PST
(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?
Comment 16 Alex Christensen 2018-02-01 13:31:38 PST
I think this patch will be a step in a good direction.
Comment 17 Michael Catanzaro 2018-02-02 17:30:34 PST
Created attachment 333021 [details]
Patch
Comment 18 Michael Catanzaro 2018-02-02 17:35:36 PST
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.
Comment 19 Michael Catanzaro 2018-02-02 17:40:51 PST
Created attachment 333022 [details]
Patch
Comment 20 Michael Catanzaro 2018-02-02 17:51:44 PST
Created attachment 333023 [details]
Patch
Comment 21 Michael Catanzaro 2018-02-03 08:59:41 PST
Comment on attachment 333023 [details]
Patch

Green! :)
Comment 22 Michael Catanzaro 2018-02-08 08:27:33 PST
(In reply to Alex Christensen from comment #16)
> I think this patch will be a step in a good direction.

r?
Comment 23 WebKit Commit Bot 2018-02-08 13:22:04 PST
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
Comment 24 Alex Christensen 2018-02-08 13:28:36 PST
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.
Comment 25 Michael Catanzaro 2018-02-08 13:38:02 PST
OK, sounds good.
Comment 26 Michael Catanzaro 2018-02-08 14:34:45 PST
Created attachment 333424 [details]
Patch
Comment 27 Michael Catanzaro 2018-02-08 15:18:33 PST
Created attachment 333428 [details]
Patch
Comment 28 Michael Catanzaro 2018-02-08 16:14:59 PST
Created attachment 333433 [details]
Patch
Comment 29 Michael Catanzaro 2018-02-08 17:32:59 PST
Committed r228304: <https://trac.webkit.org/changeset/228304>