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

[bugzilla-daemon at webkit.org]

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

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




--- Comment #29 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 362789
  --> https://bugs.webkit.org/attachment.cgi?id=362789
Patch

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

r=me with a few nits.

> Source/WebCore/loader/AdClickAttribution.h:63
> +	       : registrableDomain { RegistrableDomain { host } }

This can be made more concise and avoid an unnecessary copy which is probably
optimized out.
: registrableDomain { host }

> Source/WebCore/loader/ResourceLoadObserver.cpp:196
> -    if (url.protocolIsAbout() || url.isEmpty())
> +    if (url.protocolIsAbout() || url.isLocalFile() || url.isEmpty())

This is a change in behavior, and I don't see why it's necessary in this patch.

> Source/WebCore/platform/RegistrableDomain.h:75
> +	   if (host.length() == m_registrableDomain.length())

I would make this <= to protect against reading out of bounds below.  Otherwise
the line below needs to make sure it's reading in bounds.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.h:216
> +    const RegistrableDomain m_debugStaticPrevalentResource {
"3rdpartytestwebkit.org" };

A _s suffix on the string might help something.  It indicates it's a String and
not just a const char*.  We used to have ASCIILiteral.	Those details are
always a little unclear to me.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:66
>  using namespace WebCore;
> +using RegistrableDomain = WebCore::RegistrableDomain;

This seems redundant.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1518
> -	       process->clearPrevalentResource(m_sessionID, primaryDomain,
[processPool, callbackAggregator = callbackAggregator.copyRef()] { });
> +	       process->clearPrevalentResource(m_sessionID,
url.host().toString(), [processPool, callbackAggregator =
callbackAggregator.copyRef()] { });

This could just pass url, right?