[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