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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Feb 11 08:24:15 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 390370: Patch

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




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

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

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:302
> +	   updatePrevalentDomains(request.url());

For every sub-resource load, we may now send a
NetworkProcessProxy::UpdatePrevalentDomains() to the UIProcess. Considering
that there may be hundreds of sub-resources loads in a single page, many of
them for the same registrable domain, this seems excessive.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:342
> +    const auto domain = RegistrableDomain(url);

We usually don't use const locals in WebKit.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:413
> +	   completionHandler({ });

Shouldn't this be completionHandler(nil); ?

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:417
> +    auto webPageProxy = [webView _page];

auto*

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:419
> +	   completionHandler({ });

Shouldn't this be completionHandler(nil); ?

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:436
> +    auto webPageProxy = [webView _page];

auto*

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:521
> +void NetworkProcessProxy::updatePrevalentDomains(WebPageProxyIdentifier
pageID, RegistrableDomain domain)

RegistrableDomain&& since it is coming from IPC and you're moving it inside the
implementation.

> Source/WebKit/UIProcess/WebPageProxy.cpp:9859
> +void WebPageProxy::updatePrevalentDomains(WebCore::RegistrableDomain&&
domain)

WebCore:: is likely not needed.

> Source/WebKit/UIProcess/WebPageProxy.h:1688
> +    HashSet<WebCore::RegistrableDomain>& prevalentDomains() { return
m_prevalentDomains; }

should this be a const getter returning a const HashSet?


More information about the webkit-reviews mailing list