[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