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

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has granted
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 385564: Patch

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




--- Comment #20 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 385564
  --> https://bugs.webkit.org/attachment.cgi?id=385564
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::ThirdPartyDataForSpecificFirstPa
rty> 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.