We need a way to confirm that the NetworkProcess knows that ITP is on (or off) for testing (and perhaps other purposes). This patch adds new infrastructure to support testing this status.
<rdar://problem/51557704>
Created attachment 371703 [details] Patch
Comment on attachment 371703 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371703&action=review > Source/WebKit/NetworkProcess/NetworkProcess.cpp:656 > +void NetworkProcess::resourceLoadStatisticsEnabled(PAL::SessionID sessionID, CompletionHandler<void(bool)>&& completionHandler) At least for getters which return a boolean, we lead with an auxiliary verb. In this case, isResourceLoadStatisticsEnabled(). I don’t know if we have a rule for async boolean getters but I’d prefer a leading “is.” Ditto for the rest of the functions with the same name. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1886 > + processPool->ensureNetworkProcess().resourceLoadStatisticsEnabled(m_sessionID, WTFMove(completionHandler)); It’s a little weird to create a new network process only to check if ITP is on. We could also return that ITP is off if we don’t have a network process, but it’s probably best to do it the way you have to be able to catch race conditions where ITP was enabled before the network process existed.
Comment on attachment 371703 [details] Patch It would be even better if this test could verify that ITP still does its thing when enabled this way, instead of just relying on a network process check. I realize this makes the test even more complicated.
Comment on attachment 371703 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371703&action=review > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:483 > +- (void)_resourceLoadStatisticsEnabled:(void (^)(BOOL))completionHandler I've always been confused by what "resourceLoadStatistics" statics mean in our codebase. It seems to be a synonym for "Intelligent Tracking Prevention", but it doesn't seem like that name is used much in WebKit (I only found it in WebPreferences.yaml). Before committing this to SPI, can try to find a name that encapsulates what this feature does in a clear way and standardize on it. > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:72 > +- (void)_resourceLoadStatisticsEnabled:(void (^)(BOOL))completionHandler; Is this SPI you expect to be used outside of testing?
I think this is indeed test$only SPI, so if there is a better way to do this, we should. That would make the name less important, but I tend to agree we should call it ITP in the code instead of the “resource load statistics” fig leaf.
(In reply to Maciej Stachowiak from comment #6) > I think this is indeed test$only SPI, so if there is a better way to do > this, we should. That would make the name less important, but I tend to > agree we should call it ITP in the code instead of the “resource load > statistics” fig leaf. If this is really testing only, I think it would be much better to test that ITP is actually doing something (e.g. blocking / giving out the right cookies, etc). In which case, I don't think the new SPI is needed. Hopefully we have existing tests that of ITP functionality that could be used before and after enabling/disabling the feature.
(In reply to Sam Weinig from comment #5) > Comment on attachment 371703 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=371703&action=review > > > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:72 > > +- (void)_resourceLoadStatisticsEnabled:(void (^)(BOOL))completionHandler; > > Is this SPI you expect to be used outside of testing? No -- this is really an implementation detail that the API/SPI user shouldn't have to worry about. But we do want to test that things are staying in sync between the UIProcess and the NetworkPorcess. Maybe this would be better named "isITPEnabledInNetworkProcess" or something?
(In reply to Brent Fulgham from comment #8) > (In reply to Sam Weinig from comment #5) > > Comment on attachment 371703 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=371703&action=review > > > > > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:72 > > > +- (void)_resourceLoadStatisticsEnabled:(void (^)(BOOL))completionHandler; > > > > Is this SPI you expect to be used outside of testing? > > No -- this is really an implementation detail that the API/SPI user > shouldn't have to worry about. But we do want to test that things are > staying in sync between the UIProcess and the NetworkPorcess. > > Maybe this would be better named "isITPEnabledInNetworkProcess" or something? Our usual strategy for this kind of thing is to ensure that side effects are working. e.g. if ITP is enabled in the NetworkProcess, I see X.
Created attachment 371884 [details] Updated patch.
Created attachment 371886 [details] patch v3
Comment on attachment 371886 [details] patch v3 Rejecting attachment 371886 [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-03', 'validate-changelog', '--check-oops', '--non-interactive', 371886, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Traceback (most recent call last): File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch", line 84, in <module> main() File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch", line 79, in main WebKitPatch(os.path.abspath(__file__)).main() File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main result = command.check_arguments_and_execute(options, args, self) File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute return self.execute(options, args, tool) or 0 File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 55, in execute self._sequence.run_and_handle_errors(tool, options, state) File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors self._run(tool, options, state) File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run step(tool, options).run(state) File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 54, in run if changelog_entry.has_valid_reviewer(): AttributeError: 'NoneType' object has no attribute 'has_valid_reviewer' Full output: https://webkit-queues.webkit.org/results/12455955
Committed r246360: <https://trac.webkit.org/changeset/246360/webkit>
Comment on attachment 371886 [details] patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=371886&action=review > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:491 > +#if ENABLE(RESOURCE_LOAD_STATISTICS) > + _websiteDataStore->websiteDataStore().setPrevalentResource(URL(domain), [completionHandler = makeBlockPtr(completionHandler)]() { > + completionHandler(); > + }); > +#endif > + completionHandler(); > +} This calls the completionHandler handler twice. Once right away, once when the async function's completion handler is called.
Unreviewed fix landed in r246370: <https://trac.webkit.org/changeset/246370/webkit>
(In reply to Sam Weinig from comment #14) > Comment on attachment 371886 [details] > patch v3 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=371886&action=review > > > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:491 > > +#if ENABLE(RESOURCE_LOAD_STATISTICS) > > + _websiteDataStore->websiteDataStore().setPrevalentResource(URL(domain), [completionHandler = makeBlockPtr(completionHandler)]() { > > + completionHandler(); > > + }); > > +#endif > > + completionHandler(); > > +} > > This calls the completionHandler handler twice. Once right away, once when > the async function's completion handler is called. Doh! !@$#$#!@#