[webkit-reviews] review denied: [Bug 194791] Introduce and adopt new class RegistrableDomain for eTLD+1 : [Attachment 362683] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Feb 22 09:52:49 PST 2019


Brent Fulgham <bfulgham at webkit.org> has denied John Wilander
<wilander at apple.com>'s request for review:
Bug 194791: Introduce and adopt new class RegistrableDomain for eTLD+1
https://bugs.webkit.org/show_bug.cgi?id=194791

Attachment 362683: Patch

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




--- Comment #13 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 362683
  --> https://bugs.webkit.org/attachment.cgi?id=362683
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362683&action=review

I think this change is really, really great! I think it could be improved a bit
as suggested by Ryousuke, Fuji, and others, but it's a huge improvement.

>> Source/WebCore/loader/AdClickAttribution.h:249
>> +	encoder << m_campaign.id << m_source.registrableDomain.string() <<
m_destination.registrableDomain.string() << m_timeOfAdClick << m_conversion <<
m_earliestTimeToSend;
> 
> Is '.string()' needed? Even though RegistrableDomain has 'encode' method.

Yes, this doesn't seem necessary if RD can encode/decode itself.

>> Source/WebCore/platform/RegistrableDomain.h:40
>> +// uses the full domain for ports that haven't enabled PUBLIC_SUFFIX_LIST.
> 
> In particular, this is the kind of information you can directly get from
reading code, and tends to get outdated rather quickly.

This would be good information in the ChangeLog, though.

> Source/WebCore/platform/RegistrableDomain.h:47
> +	   : m_registrableDomain {
topPrivatelyControlledDomain(url.host().toString()) }

Is this right? Our old code did this:

    auto domain = topPrivatelyControlledDomain(url.host().toString());
    if (domain.isEmpty())
	domain = url.host().toString();

>> Source/WebCore/platform/RegistrableDomain.h:67
>> +	bool operator!=(const RegistrableDomain& other) const { return
m_registrableDomain != other.m_registrableDomain; }
> 
> I want a method to compare with URL, for example 'matches'.
> Can it be implemented by using StringView.endsWith?
> 
> bool matches(URL url) {
>   auto host = url.host();
>   if (!host.endsWith(m_registrableDomain))
>     return fasle;
>    if (host.length() == m_registrableDomain.length())
>    return true;
>   return host[host.length() - m_registrableDomain.length()] == '.'
> }

This does seem like a good idea, especially if we can avoid converting the
entire URL into a temporary string just to compare the host portion..

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:2
55
> +	       workQueue->dispatch([domainsWithDeletedWebsiteData =
crossThreadCopy(domainsWithDeletedWebsiteData), callback = WTFMove(callback),
weakThis = WTFMove(weakThis)] () mutable {

It seems like both tiers of lambdas here should work in terms of HashSets of
RegistrableDomains, rather than strings that have to be converted to
RegistrableDomains later.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:2
60
> +		   for (auto& domain : domainsWithDeletedWebsiteData) {

Much better naming!

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:2
89
> +	   auto mapEntry = m_resourceStatisticsMap.find(RegistrableDomain {
subresourceUniqueRedirectFromDomain });

We should make 'resourceStatistic.subresourceUniqueRedirectsFrom' (and the
others) containers of RegistrableDomain when they are serialized out of the
plist. Then you don't need to do this conversion and string copying every time
you loop or search.

Likewise, m_resourceStatisticsMap should be a container keyed by
RegistrableDomain for the same reason.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.h:58
> +    using SubFrameDomain = WebCore::RegistrableDomain;

Great!

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.h:222
> +

Extra whitespace

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:63
> +    using ResourceLoadStatistics = WebCore::ResourceLoadStatistics;

I really like this approach. I wonder if all of this code would be better/less
error prone if instead of 'using' you actually had subclasses for each of these
concepts. This would prevent us from accidentally mixing up the TopFrameDomain
and SubFrameDomain when passing to a helper function.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:-622
> -void
WebsiteDataStore::fetchDataForTopPrivatelyControlledDomains(OptionSet<WebsiteDa
taType> dataTypes, OptionSet<WebsiteDataFetchOption> fetchOptions, const
Vector<String>& topPrivatelyControlledDomains,
Function<void(Vector<WebsiteDataRecord>&&, HashSet<String>&&)>&&
completionHandler)

Wow! Was this never used?


More information about the webkit-reviews mailing list