[Webkit-unassigned] [Bug 65564] Support for multiple <link rel="icon"> favicon elements.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=65564


Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #107083|review?                     |review-
               Flag|                            |




--- Comment #28 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2011-09-16 14:46:57 PST ---
(From update of attachment 107083)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.