Bug 204932 – Create WebKit API calls for ITP Data

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

The data captured in the ITP store needs to be exposed to WebKit clients through the Objective C API.

[Kate Cheney]

<rdar://problem/57632753>

[Kate Cheney]

Created attachment 384982 [details]
Patch

[Kate Cheney]

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.

[Sam Weinig]

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.

[Kate Cheney]

Created attachment 385166 [details]
Patch

[Kate Cheney]

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

[Kate Cheney]

Created attachment 385176 [details]
Patch

[Kate Cheney]

Changed header files to be Project, this should fix the iOS compile failures.

[Sam Weinig]

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.

[Kate Cheney]

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

[Sam Weinig]

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

[Kate Cheney]

Created attachment 385220 [details]
Patch

[Alex Christensen]

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.

[Kate Cheney]

Created attachment 385449 [details]
Patch

[Kate Cheney]

Mac-wk2 failure is for webrtc and seems unrelated

[Brent Fulgham]

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

[Alex Christensen]

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.

[Kate Cheney]

Created attachment 385564 [details]
Patch

[Kate Cheney]

Thanks Alex!

[Alex Christensen]

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.

[Kate Cheney]

Created attachment 385609 [details]
Patch

[Kate Cheney]

(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

[WebKit Commit Bot]

Comment on attachment 385609 [details]
Patch

Clearing flags on attachment: 385609

Committed r253484: <https://trac.webkit.org/changeset/253484>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57915968>

[Alex Christensen]

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.

[Kate Cheney]

(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