Bug 195637 – Drop legacy WebCore::toRegistrableDomain() utility function

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Drop legacy WebCore::toRegistrableDomain() utility function. Call sites should be constructing a WebCore::RegistrableDomain instead.

[Chris Dumez]

Created attachment 364436 [details]
Patch

[Chris Dumez]

Needs https://bugs.webkit.org/show_bug.cgi?id=195634 to land first.

[John Wilander]

Comment on attachment 364436 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:3453
> +    if (targetDocument && (targetDocument->securityOrigin().canAccess(SecurityOrigin::create(destinationURL)) || RegistrableDomain { targetDocument->url() } == RegistrableDomain { destinationURL }))

Is there a reason why you prefer creating two objects over comparing one with the matches() function?

> Source/WebCore/loader/CrossOriginAccessControl.cpp:233
> +        if (RegistrableDomain { response.url() } != RegistrableDomain::uncheckedCreateFromHost(origin.host()))

Ditto. You have the response URL.

> Source/WebCore/loader/FrameLoader.cpp:1112
> +        if (SecurityPolicy::shouldInheritSecurityOriginFromOwner(frame->document()->url()) || RegistrableDomain { frame->document()->url() } == registrableDomain)

Ditto.

> Source/WebCore/loader/FrameLoader.cpp:2973
> +    request.setIsSameSite(RegistrableDomain { initiator->siteForCookies() } == RegistrableDomain { request.url() });

Ditto.

> Source/WebCore/platform/network/ResourceRequestBase.h:-260
> -// FIXME: Find a better place for these functions.

Yay, a FIXME gets fixed.

> Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp:289
> +    m_sameSiteDisposition = !siteForCookies ? SameSiteDisposition::Unspecified : (RegistrableDomain { siteForCookies.get() } ==  RegistrableDomain { m_url }) ? SameSiteDisposition::SameSite : SameSiteDisposition::CrossSite);

Here you could also use the faster matches() function.

> Source/WebCore/platform/network/cocoa/ResourceRequestCocoa.mm:92
> +    m_sameSiteDisposition = siteForCookies.isNull() ? SameSiteDisposition::Unspecified : ((RegistrableDomain { siteForCookies } == RegistrableDomain { m_url }) ? SameSiteDisposition::SameSite : SameSiteDisposition::CrossSite);

Ditto.

> Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:148
> +    return WebCore::RegistrableDomain { request.url() } != request.firstPartyForCookies();

If request.firstPartyForCookies() returns a RegistrableDomain, you could use matches() here too.

> Source/WebKit/UIProcess/WebProcessPool.cpp:1312
> +            updatedRequest.setIsSameSite(RegistrableDomain { initiatingPageURL } == RegistrableDomain { request.url() });

Can use matches().

> Source/WebKit/UIProcess/WebProcessPool.cpp:2299
> +    if (!sourceURL.isValid() || !targetURL.isValid() || sourceURL.isEmpty() || sourceURL.protocolIsAbout() || RegistrableDomain { sourceURL } == targetRegistrableDomain)

Ditto.

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:398
> +    if (m_hasFrameSpecificStorageAccess && RegistrableDomain { currentUrl } != RegistrableDomain { newUrl }) {

Ditto.

[Chris Dumez]

(In reply to John Wilander from comment #3)
> Comment on attachment 364436 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364436&action=review
> 
> > Source/WebCore/dom/Document.cpp:3453
> > +    if (targetDocument && (targetDocument->securityOrigin().canAccess(SecurityOrigin::create(destinationURL)) || RegistrableDomain { targetDocument->url() } == RegistrableDomain { destinationURL }))
> 
> Is there a reason why you prefer creating two objects over comparing one
> with the matches() function?

I merely did not know about it. I'll update the patch, thanks for looking.

[Chris Dumez]

Created attachment 364447 [details]
Patch

[Chris Dumez]

Created attachment 364549 [details]
Patch

[Geoffrey Garen]

Comment on attachment 364549 [details]
Patch

r=me

[Geoffrey Garen]

Comment on attachment 364549 [details]
Patch

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

> Source/WebCore/platform/RegistrableDomain.h:134
> +inline bool registrableDomainsAreEqual(const URL& a, const URL& b)

Nit: I would name this areRegistrableDomainsEqual. I think seeing is/are as a prefix helps show that what follows is a boolean condition, and I think that's how Apple API is typically written.

[Chris Dumez]

Created attachment 364554 [details]
Patch

[Chris Dumez]

Created attachment 364559 [details]
Patch

[Chris Dumez]

Comment on attachment 364559 [details]
Patch

Clearing flags on attachment: 364559

Committed r242899: <https://trac.webkit.org/changeset/242899>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48859177>