Summary: | Create WebKit API calls for ITP Data | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kate Cheney <katherine_cheney> | ||||||||||||||||
Component: | WebKit Misc. | Assignee: | Kate Cheney <katherine_cheney> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | achristensen, bfulgham, commit-queue, sam, webkit-bug-importer, wilander | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Kate Cheney
2019-12-05 16:57:04 PST
Created attachment 384982 [details]
Patch
Style bot will be angry because it doesn't recognize that "WKWebsiteDataStoreInternal.h" is the header file for WKWebsiteDataStore.mm, so it thinks the alphabetic order is wrong. Comment on attachment 384982 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384982&action=review > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:74 > +- (void)_getITPDataSummary:(void (^)(NSArray<NSDictionary *> *thirdPartyDomains))completionHandler WK_API_AVAILABLE(macos(10.15), ios(13.0)); Can we avoid the term ITP here? It's pretty unclear (at least to me) what that means, and is not a term we use in the code with much consistency (most places seem to refer to resource load statistics). Also, rather than returning a dictionary, it seems like a strongly typed objective-c type would be preferable unless there is a really compelling reason this needs to be a dictionary. Created attachment 385166 [details]
Patch
(In reply to Sam Weinig from comment #4) > Comment on attachment 384982 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=384982&action=review > > > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:74 > > +- (void)_getITPDataSummary:(void (^)(NSArray<NSDictionary *> *thirdPartyDomains))completionHandler WK_API_AVAILABLE(macos(10.15), ios(13.0)); > > Can we avoid the term ITP here? It's pretty unclear (at least to me) what > that means, and is not a term we use in the code with much consistency (most > places seem to refer to resource load statistics). > Agreed for consistency it makes sense to use resource load statistics, thanks for pointing this out. > Also, rather than returning a dictionary, it seems like a strongly typed > objective-c type would be preferable unless there is a really compelling > reason this needs to be a dictionary. Good idea! The patch I just uploaded adds two new classes to hold the data instead of an NS Dictionary. Created attachment 385176 [details]
Patch
Changed header files to be Project, this should fix the iOS compile failures. Comment on attachment 385176 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385176&action=review > Source/WebKit/UIProcess/API/Cocoa/WKITPThirdParty.h:31 > +@interface WKITPThirdParty : NSObject Nice. I like the concrete class much better. However, kike with the method, I think it would make sense to stay consistent here and not use ITP in the name. Also, since this is being exposed as SPI, this class should start with an _. (Both comments apply to WKITPFirstParty as well). > Source/WebKit/UIProcess/API/Cocoa/WKITPThirdParty.h:34 > +- (NSArray<WKITPFirstParty *> *)getWKITPFirstPartyList:(Vector<WebKit::WebResourceLoadStatisticsStore::ThirdPartyDataForSpecificFirstParty> *)underFirstParties; > +- (instancetype)initWithDomain:(NSString *)domain underFirstParties:(Vector<WebKit::WebResourceLoadStatisticsStore::ThirdPartyDataForSpecificFirstParty>&)underFirstParties; It's not valid to reference WebKit c++ classes from WebKit API/SPI headers. The model we use is to have a WKITPThirdPartyInternal.h that exposes this, and that header is not present in the SDK. (In reply to Sam Weinig from comment #9) > Comment on attachment 385176 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=385176&action=review > > > Source/WebKit/UIProcess/API/Cocoa/WKITPThirdParty.h:34 > > +- (NSArray<WKITPFirstParty *> *)getWKITPFirstPartyList:(Vector<WebKit::WebResourceLoadStatisticsStore::ThirdPartyDataForSpecificFirstParty> *)underFirstParties; > > +- (instancetype)initWithDomain:(NSString *)domain underFirstParties:(Vector<WebKit::WebResourceLoadStatisticsStore::ThirdPartyDataForSpecificFirstParty>&)underFirstParties; > > It's not valid to reference WebKit c++ classes from WebKit API/SPI headers. > The model we use is to have a WKITPThirdPartyInternal.h that exposes this, > and that header is not present in the SDK. Ah okay, good to know. Should WKITPThirdPartyInternal.h also have an underscore before it? (In reply to katherine_cheney from comment #10) > (In reply to Sam Weinig from comment #9) > > Comment on attachment 385176 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=385176&action=review > > > > > Source/WebKit/UIProcess/API/Cocoa/WKITPThirdParty.h:34 > > > +- (NSArray<WKITPFirstParty *> *)getWKITPFirstPartyList:(Vector<WebKit::WebResourceLoadStatisticsStore::ThirdPartyDataForSpecificFirstParty> *)underFirstParties; > > > +- (instancetype)initWithDomain:(NSString *)domain underFirstParties:(Vector<WebKit::WebResourceLoadStatisticsStore::ThirdPartyDataForSpecificFirstParty>&)underFirstParties; > > > > It's not valid to reference WebKit c++ classes from WebKit API/SPI headers. > > The model we use is to have a WKITPThirdPartyInternal.h that exposes this, > > and that header is not present in the SDK. > > Ah okay, good to know. Should WKITPThirdPartyInternal.h also have an > underscore before it? Yup. Take a look at _WKFrameHandle{Internal}.h/mm as a reference. Created attachment 385220 [details]
Patch
Comment on attachment 385220 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385220&action=review > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1166 > + Vector<WebResourceLoadStatisticsStore::ThirdPartyData> thirdPartyData = m_statisticsStore->aggregatedThirdPartyData(); I think this could be auto. > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:532 > + completionHandler(WTFMove(apiThirdParties)); It's a little strange to WTFMove an ObjC pointer. Let's not. > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:535 > + completionHandler({ }); This should be nil. > Source/WebKit/UIProcess/API/Cocoa/_WKResourceLoadStatisticsFirstParty.h:2 > + * Copyright (C) 2017 Apple Inc. All rights reserved. 2019 > Source/WebKit/UIProcess/API/Cocoa/_WKResourceLoadStatisticsFirstParty.mm:2 > + * Copyright (C) 2015 Apple Inc. All rights reserved. 2019. All your copyright dates need updating. > Source/WebKit/UIProcess/API/Cocoa/_WKResourceLoadStatisticsFirstParty.mm:37 > + self.firstPartyDomain = domain; > + self.thirdPartyStorageAccessGranted = storageAccess; Please make these wrappers of C++ objects like all our other API objects. See WKWebpagePreferences for an example. Created attachment 385449 [details]
Patch
Mac-wk2 failure is for webrtc and seems unrelated (In reply to katherine_cheney from comment #15) > Mac-wk2 failure is for webrtc and seems unrelated I agree. The EWS system seems to have some kind of media stack issue that is causing timeouts. Comment on attachment 385449 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385449&action=review Getting closer! > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:125 > + WebCore::RegistrableDomain decodedDomain; It would be a little bit more consistent to do this here: Optional<RegistrableDomain> decodedDomain; decoder >> decodedDomain; if (!decodedDomain) return WTF::nullopt; ... return {{ WTFMove(*decodedDomain), ... }}; Our IPC code is in a strange state right now, but that's most like what we're trying to get to. Same with other decoders in this patch. > Source/WebKit/UIProcess/API/APIResourceLoadStatisticsFirstParty.h:33 > +class ResourceLoadStatisticsFirstParty final : public API::ObjectImpl<API::Object::Type::ResourceLoadStatisticsFirstParty> { The API::'s are unnecessary because we're inside namespace API here. > Source/WebKit/UIProcess/API/APIResourceLoadStatisticsThirdParty.h:40 > + Vector<WebKit::WebResourceLoadStatisticsStore::ThirdPartyDataForSpecificFirstParty> underFirstParties() { return m_underFirstParties; } const, const > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:528 > + _WKResourceLoadStatisticsThirdParty *wkThirdParty = [[_WKResourceLoadStatisticsThirdParty alloc] init]; You shouldn't need to create a new _WKResourceLoadStasticsThirdParty object here because wrapper(thirdParty) is a _WKResourceLoadStatisticsThirdParty. Just move them from the Vector to the NSArray. > Source/WebKit/UIProcess/API/Cocoa/_WKResourceLoadStatisticsFirstParty.h:30 > +@property (nonatomic, copy, setter=_setFirstPartyDomain:) NSString *_firstPartyDomain; readonly? > Source/WebKit/UIProcess/API/Cocoa/_WKResourceLoadStatisticsThirdParty.h:33 > +@property (nonatomic, copy, setter=_setThirdPartyDomain:) NSString *_thirdPartyDomain; These don't need underscores because they are properties on a class that starts with an underscore. Can these properties be made readonly? > Source/WebKit/UIProcess/API/Cocoa/_WKResourceLoadStatisticsThirdParty.h:36 > +- (NSArray<_WKResourceLoadStatisticsFirstParty *> *)_getWKResourceLoadStatisticsFirstPartyList:(Vector<WebKit::WebResourceLoadStatisticsStore::ThirdPartyDataForSpecificFirstParty> *)underFirstParties; This should not be in an API header because it uses a WTF type, Vector. For this functionally you should just call a C++ function through _thirdParty inside a .mm file. > Source/WebKit/UIProcess/API/Cocoa/_WKResourceLoadStatisticsThirdParty.mm:39 > + _WKResourceLoadStatisticsFirstParty *wkFirstParty = [[_WKResourceLoadStatisticsFirstParty alloc] init]; Ditto, no copying and setting here. Created attachment 385564 [details]
Patch
Thanks Alex! Comment on attachment 385564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385564&action=review r=me once these nits are addressed. > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:184 > + bool operator<(const ThirdPartyData &other) const I think in a future patch we should replace this with a comparator at a call site of std::sort. It's not inherent to this type to have a less than comparison, it's just how we want to sort certain containers. > Source/WebKit/UIProcess/API/APIResourceLoadStatisticsFirstParty.h:47 > + const WTF::String& firstPartyDomain() { return m_firstPartyData.firstPartyDomain.string(); } > + bool storageAccess() { return m_firstPartyData.storageAccessGranted; } const after parentheses because it doesn't mutate the object to call these functions, and they return a non-mutable reference or POD type. > Source/WebKit/UIProcess/API/APIResourceLoadStatisticsFirstParty.h:50 > + WebKit::WebResourceLoadStatisticsStore::ThirdPartyDataForSpecificFirstParty m_firstPartyData; const? > Source/WebKit/UIProcess/API/APIResourceLoadStatisticsThirdParty.h:36 > + static Ref<ResourceLoadStatisticsThirdParty> create(const WebKit::WebResourceLoadStatisticsStore::ThirdPartyData thirdPartyData) This could probably take a ThirdPartyData&& then move it into the constructor, which moves it into the member variable. > Source/WebKit/UIProcess/API/APIResourceLoadStatisticsThirdParty.h:47 > + Vector<WebKit::WebResourceLoadStatisticsStore::ThirdPartyDataForSpecificFirstParty> underFirstParties() { return m_thirdPartyData.underFirstParties; } This is an unnecessary implicit Vector copy. If you made this return a const Vector<...>& it would remove the copy. > Source/WebKit/UIProcess/API/APIResourceLoadStatisticsThirdParty.h:50 > + WebKit::WebResourceLoadStatisticsStore::ThirdPartyData m_thirdPartyData; This could probably be marked const because it's not mutated after construction. Created attachment 385609 [details]
Patch
(In reply to Alex Christensen from comment #20) > Comment on attachment 385564 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=385564&action=review > > r=me once these nits are addressed. > Thanks again for all the feedback! > > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:184 > > + bool operator<(const ThirdPartyData &other) const > > I think in a future patch we should replace this with a comparator at a call > site of std::sort. It's not inherent to this type to have a less than > comparison, it's just how we want to sort certain containers. > Good idea, I can file a Radar Comment on attachment 385609 [details] Patch Clearing flags on attachment: 385609 Committed r253484: <https://trac.webkit.org/changeset/253484> All reviewed patches have been landed. Closing bug. Comment on attachment 385609 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385609&action=review > Source/WebKit/UIProcess/API/Cocoa/_WKResourceLoadStatisticsFirstParty.h:29 > + These should probably have this: + (instancetype)new NS_UNAVAILABLE; - (instancetype)init NS_UNAVAILABLE; That will prevent people from alloc init'ing these. (In reply to Alex Christensen from comment #26) > Comment on attachment 385609 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=385609&action=review > > > Source/WebKit/UIProcess/API/Cocoa/_WKResourceLoadStatisticsFirstParty.h:29 > > + > > These should probably have this: > > + (instancetype)new NS_UNAVAILABLE; > - (instancetype)init NS_UNAVAILABLE; > > That will prevent people from alloc init'ing these. https://bugs.webkit.org/show_bug.cgi?id=205221 |