[webkit-reviews] review denied: [Bug 65564] Support for multiple <link rel="icon"> favicon elements. : [Attachment 107083] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Rachel Blum
<groby at chromium.org>'s request for review:
Bug 65564: Support for multiple <link rel="icon"> favicon elements.
https://bugs.webkit.org/show_bug.cgi?id=65564

Attachment 107083: Patch
https://bugs.webkit.org/attachment.cgi?id=107083&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=107083&action=review


> Source/WebCore/dom/Document.cpp:4372
> +	 IconURL favIconURL = f->loader()->icon()->iconURL(iconType);

nit: indentation should be 4 white spaces

> Source/WebCore/dom/Document.cpp:4373
> +	 if (favIconURL == newURL)

why do you need to restrict calls to didChangeIcons?  also, why is this
variable
named favIconURL?  couldn't iconType indicate a different type of icon?

> Source/WebCore/dom/IconURL.h:59
>	   : m_iconType(InvalidIcon)

this should initialize m_isDefaultIcon

> Source/WebCore/dom/IconURL.h:72
> +    static IconURL DefaultIconURL(const KURL&, IconType);

nit: method names start with a lower case letter

> Source/WebCore/loader/icon/IconController.cpp:120
> +	       iconURLs.append(*iter);

should you be increasing iconCount here since iconURLs.size() is increasing?