Bug 195637 - Drop legacy WebCore::toRegistrableDomain() utility function
Summary: Drop legacy WebCore::toRegistrableDomain() utility function
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 195634
Blocks:
  Show dependency treegraph
 
Reported: 2019-03-12 12:50 PDT by Chris Dumez
Modified: 2019-03-13 12:54 PDT (History)
12 users (show)

See Also:


Attachments
Patch (19.40 KB, patch)
2019-03-12 13:21 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (11.63 KB, patch)
2019-03-12 14:41 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (11.63 KB, patch)
2019-03-13 10:55 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (17.24 KB, patch)
2019-03-13 11:44 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (17.24 KB, patch)
2019-03-13 12:06 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2019-03-12 12:50:35 PDT
Drop legacy WebCore::toRegistrableDomain() utility function. Call sites should be constructing a WebCore::RegistrableDomain instead.
Comment 1 Chris Dumez 2019-03-12 13:21:44 PDT
Created attachment 364436 [details]
Patch
Comment 2 Chris Dumez 2019-03-12 13:22:41 PDT
Needs https://bugs.webkit.org/show_bug.cgi?id=195634 to land first.
Comment 3 John Wilander 2019-03-12 13:47:12 PDT
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.
Comment 4 Chris Dumez 2019-03-12 13:54:40 PDT
(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.
Comment 5 Chris Dumez 2019-03-12 14:41:22 PDT
Created attachment 364447 [details]
Patch
Comment 6 Chris Dumez 2019-03-13 10:55:38 PDT
Created attachment 364549 [details]
Patch
Comment 7 Geoffrey Garen 2019-03-13 10:57:21 PDT
Comment on attachment 364549 [details]
Patch

r=me
Comment 8 Geoffrey Garen 2019-03-13 10:58:43 PDT
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.
Comment 9 Chris Dumez 2019-03-13 11:44:49 PDT
Created attachment 364554 [details]
Patch
Comment 10 Chris Dumez 2019-03-13 12:06:48 PDT
Created attachment 364559 [details]
Patch
Comment 11 Chris Dumez 2019-03-13 12:53:55 PDT
Comment on attachment 364559 [details]
Patch

Clearing flags on attachment: 364559

Committed r242899: <https://trac.webkit.org/changeset/242899>
Comment 12 Chris Dumez 2019-03-13 12:53:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2019-03-13 12:54:32 PDT
<rdar://problem/48859177>