Bug 209032

<rdar://problem/60397323>

Created attachment 393438 [details]
Patch

Created attachment 393441 [details]
Patch

Comment on attachment 393441 [details]
Patch

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

Bots seem unhappy.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:393
> +- (void)_getLoadedDomainsFor:(WKWebView *)webView completionHandler:(void (^)(NSArray<NSString *> *domains))completionHandler

If this truly returns third-party loads I think the name should include that, _getLoadedThirdPartyDomains(). Likewise for the rest of the re-naming.

> Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:348
> +    void getLoadedDomains(object callback);

Likewise for re-naming in Tools code.

Build failure in WebPage.cpp.

(In reply to John Wilander from comment #5)
> Build failure in WebPage.cpp.

(In reply to John Wilander from comment #4)
> Comment on attachment 393441 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393441&action=review
> 
> Bots seem unhappy.
> 

Whoops, not sure why that wasn't caught by my local build

> > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:393
> > +- (void)_getLoadedDomainsFor:(WKWebView *)webView completionHandler:(void (^)(NSArray<NSString *> *domains))completionHandler
> 
> If this truly returns third-party loads I think the name should include
> that, _getLoadedThirdPartyDomains(). Likewise for the rest of the re-naming.
> 
> > Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:348
> > +    void getLoadedDomains(object callback);
> 
> Likewise for re-naming in Tools code.

Good call, uploading a new patch with this change.

Created attachment 393483 [details]
Patch

(In reply to katherine_cheney from comment #7)
> Created attachment 393483 [details]
> Patch

Style bot always seems upset when adding a function to Source/WebCore/loader/FrameLoaderClient.h :(

Comment on attachment 393483 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebPage.h:1192
> +    void getLoadedThirdPartyDomains(CompletionHandler<void(Vector<WebCore::RegistrableDomain>)>&&);

No get prefixes for getters please.

Comment on attachment 393483 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6700
> +    m_loadedThirdPartyDomains.add(domain);

Where in your code are you checking that it is a third-party domain? As far as I can tell, you are adding the first-party domain to this set too.

(In reply to Chris Dumez from comment #10)
> Comment on attachment 393483 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393483&action=review
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6700
> > +    m_loadedThirdPartyDomains.add(domain);
> 
> Where in your code are you checking that it is a third-party domain? As far
> as I can tell, you are adding the first-party domain to this set too.

You're right. I think I need to add a check for if the mainFrameURL == targetURL

(In reply to katherine_cheney from comment #11)
> (In reply to Chris Dumez from comment #10)
> > Comment on attachment 393483 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=393483&action=review
> > 
> > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6700
> > > +    m_loadedThirdPartyDomains.add(domain);
> > 
> > Where in your code are you checking that it is a third-party domain? As far
> > as I can tell, you are adding the first-party domain to this set too.
> 
> You're right. I think I need to add a check for if the mainFrameURL ==
> targetURL

I believe you want to be checking registrable domains, not URLs.

(In reply to Chris Dumez from comment #12)
> (In reply to katherine_cheney from comment #11)
> > (In reply to Chris Dumez from comment #10)
> > > Comment on attachment 393483 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=393483&action=review
> > > 
> > > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6700
> > > > +    m_loadedThirdPartyDomains.add(domain);
> > > 
> > > Where in your code are you checking that it is a third-party domain? As far
> > > as I can tell, you are adding the first-party domain to this set too.
> > 
> > You're right. I think I need to add a check for if the mainFrameURL ==
> > targetURL
> 
> I believe you want to be checking registrable domains, not URLs.

Yes, typo. I will be comparing the RegistrableDomains in WebPage::addLoadedThirdPartyDomain before adding to the HashSet. Thank you for pointing this out!

Created attachment 393494 [details]
Patch

(In reply to katherine_cheney from comment #11)
> (In reply to Chris Dumez from comment #10)
> > Comment on attachment 393483 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=393483&action=review
> > 
> > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6700
> > > +    m_loadedThirdPartyDomains.add(domain);
> > 
> > Where in your code are you checking that it is a third-party domain? As far
> > as I can tell, you are adding the first-party domain to this set too.
> 
> You're right. I think I need to add a check for if the mainFrameURL ==
> targetURL

I actually thought about this but reckoned that it must be checked already since the same bug would otherwise apply to existing code with the isPrevalent check. Apparently we do have that bug in trunk then. Good catch, Chris!

Comment on attachment 393494 [details]
Patch

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

> Source/WebCore/Modules/websockets/WebSocket.cpp:331
> +            frame->loader().client().addLoadedThirdPartyDomain(WTFMove(domain));

You're calling addLoadedThirdPartyDomain() with a domain that is not necessarily third-party. As a result, I think this method is badly named. Maybe something like FrameLoaderClient::didLoadFromRegistrableDomain(), very generic and then it is up to the client implementation to decide what to do with it.

Created attachment 393513 [details]
Patch for landing

(In reply to Chris Dumez from comment #16)
> Comment on attachment 393494 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393494&action=review
> 
> > Source/WebCore/Modules/websockets/WebSocket.cpp:331
> > +            frame->loader().client().addLoadedThirdPartyDomain(WTFMove(domain));
> 
> You're calling addLoadedThirdPartyDomain() with a domain that is not
> necessarily third-party. As a result, I think this method is badly named.
> Maybe something like FrameLoaderClient::didLoadFromRegistrableDomain(), very
> generic and then it is up to the client implementation to decide what to do
> with it.

Made these changes, thanks Chris!

Comment on attachment 393513 [details]
Patch for landing

Clearing flags on attachment: 393513

Committed r258421: <https://trac.webkit.org/changeset/258421>

All reviewed patches have been landed.  Closing bug.

<rdar://problem/60429255>