[webkit-reviews] review granted: [Bug 174073] Switch all WebKit API related to favicons from WebIconDatabase over to new WebCore::IconLoader mechanism : [Attachment 314481] Patch

[bugzilla-daemon at webkit.org]

Andy Estes <aestes at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 174073: Switch all WebKit API related to favicons from WebIconDatabase over
to new WebCore::IconLoader mechanism
https://bugs.webkit.org/show_bug.cgi?id=174073

Attachment 314481: Patch

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




--- Comment #37 from Andy Estes <aestes at apple.com> ---
Comment on attachment 314481
  --> https://bugs.webkit.org/attachment.cgi?id=314481
Patch

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

> Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:2283
> +    auto* frame = core(m_webFrame.get());
> +    DocumentLoader* documentLoader = frame->loader().documentLoader();
> +    ASSERT(documentLoader);

Can these be references instead of pointers?

> Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:2289
> +    if (!frame->isMainFrame() ||
!documentLoader->url().protocolIsInHTTPFamily() || ![WebView
_isIconLoadingEnabled] || disallowedDueToImageLoadSettings) {

I'd rewrite this to check disallowedDueToImageLoadSettings first.

> Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:2299
> +    for (auto icon = icons.rbegin(); icon != icons.rend(); ++icon) {

It's not clear to me why we have to iterate backwards; I'm sure there's a
reason.

> Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:2311
> +    // No WebCore icon loading on iOS
> +    for (auto& icon : icons)
> +	   documentLoader->didGetLoadDecisionForIcon(false, icon.second, 0);

Can iOS be handled like the other cases where icon loading is disallowed?

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1794
> +    for (auto& icon : icons)
> +	  
webPage->send(Messages::WebPageProxy::GetLoadDecisionForIcon(icon.first,
CallbackID::fromInteger(icon.second)));

Can this be sent as one message containing all the icons instead of a message
for each icon?

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.h:260
> +    void getLoadDecisionForIcons(const Vector<std::pair<WebCore::LinkIcon&,
uint64_t>>&) final;

The vector of pairs could use a typedef.