[webkit-reviews] review denied: [Bug 207523] Expose prevalent domains on a per-page basis : [Attachment 390580] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Feb 13 14:55:40 PST 2020


Chris Dumez <cdumez at apple.com> has denied katherine_cheney at apple.com's request
for review:
Bug 207523: Expose prevalent domains on a per-page basis
https://bugs.webkit.org/show_bug.cgi?id=207523

Attachment 390580: Patch

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




--- Comment #24 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 390580
  --> https://bugs.webkit.org/attachment.cgi?id=390580
Patch

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

Almost perfect, just a few more improvements.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:794
> +   
frame.loader().client().addLoadedRegistrableDomain(RegistrableDomain(request.re
sourceRequest().url()));

You may need something in CachedResource::redirectReceived() too to handle
redirects. Please double check to Youenn where is the best place to add the
addLoadedRegistrableDomain() for handling redirect cases.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:791
> +void
NetworkConnectionToWebProcess::isPrevalentSubresourceLoad(WebPageProxyIdentifie
r webPageProxyID, RegistrableDomain&& domain, CompletionHandler<void(bool)>&&
completionHandler)

webPageProxyID parameter is unused and should be dropped.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:801
> +#endif

Please add a blank line after this one.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:423
> +#if ENABLE(RESOURCE_LOAD_STATISTICS)

Why isn't this the first line of this method?

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:424
> +    webPageProxy->getPrevalentDomains([completionHandler =
makeBlockPtr(completionHandler)] (HashSet<WebCore::RegistrableDomain>
prevalentDomains) {

HashSet<WebCore::RegistrableDomain>&&

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:428
> +	      
apiDomains.uncheckedAppend(API::String::create(domain.string()));

WTFMove(domain.string())

but you will need to add this to RegistrableDomain:
String& string() { return m_registrableDomain; }

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:445
> +#if ENABLE(RESOURCE_LOAD_STATISTICS)

Why isn't this the first line of this method?

> Source/WebKit/UIProcess/WebPageProxy.cpp:4567
> +	   clearPrevalentDomains();

This is weird, it should be in the didCommitLoadForFrame() in the WebProcess
instead, now that we don't store them in the UIProcess. It does unnecessary IPC
otherwise.

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:159
> +    WebPage* webPage = m_frame->page();

auto*

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6593
> +    clearPrevalentDomains();

Not needed anymore now that things are only stored in the WebProcess. You want
this in didCommitLoadForFrame() when we are the main frame though (in
WebProcess side).

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6673
> +void WebPage::addLoadedRegistrableDomain(WebCore::RegistrableDomain&&
domain)

I doubt WebCore:: is really needed.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6677
> +	  
WebProcess::singleton().ensureNetworkProcessConnection().connection().sendWithA
syncReply(Messages::NetworkConnectionToWebProcess::IsPrevalentSubresourceLoad(m
_webPageProxyIdentifier, domain), [this, protectedThis = makeRefPtr(*this),
domain] (bool isPrevalent) {

m_webPageProxyIdentifier parameter is unnecessary.


More information about the webkit-reviews mailing list