[webkit-reviews] review denied: [Bug 204932] Create WebKit API calls for ITP Data : [Attachment 385449] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied
katherine_cheney at apple.com's request for review:
Bug 204932: Create WebKit API calls for ITP Data
https://bugs.webkit.org/show_bug.cgi?id=204932

Attachment 385449: Patch

https://bugs.webkit.org/attachment.cgi?id=385449&action=review




--- Comment #17 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 385449
  --> https://bugs.webkit.org/attachment.cgi?id=385449
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::ThirdPartyDataForSpecificFirstPa
rty> 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
> + at property (nonatomic, copy, setter=_setFirstPartyDomain:) NSString
*_firstPartyDomain;

readonly?

> Source/WebKit/UIProcess/API/Cocoa/_WKResourceLoadStatisticsThirdParty.h:33
> + at 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::WebResourceLoadSta
tisticsStore::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.