[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 #106088|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #25 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2011-09-07 15:00:24 PST ---
(From update of attachment 106088)
View in context: https://bugs.webkit.org/attachment.cgi?id=106088&action=review

> Source/WebCore/dom/Document.cpp:4368
> +    IconURL newURL(KURL(ParsedURLString, url), sizes, mimeType, iconType, false);

nit: mystery true/false literals at callsites are ordinarily frowned upon in webkit code.
it makes the code less readable since you need to consult the class definition to see
what this parameter means.  it is preferred to use enums or have separately named
functions.

> Source/WebCore/dom/Document.h:921
> +    void addIconURL(const String&, const String&, const String&, IconType);

please name the first and second parameters.  it is not obvious what they are
supposed to represent.  the only time we drop the parameter name is when the
parameter type is self-documenting, as is the case with the IconType parameter.

> Source/WebCore/dom/IconURL.h:56
> +    bool m_isDefault;

where is m_isDefault used?  i don't see it referenced elsewhere in this patch (aside
from the operator== implementation).

> Source/WebCore/dom/IconURL.h:64
> +    : m_iconType(type)

nit: indent member initializers

> Source/WebCore/dom/IconURL.h:73
> +bool operator==(const IconURL&, const IconURL&);

nit: add a new line below here

> Source/WebCore/loader/DocumentLoader.cpp:-685
> -IconURL DocumentLoader::iconURL(IconType iconType) const

nice removal!

> Source/WebCore/loader/icon/IconController.cpp:76
> +                result=*iter;

nit: add whitespace around the "=" operator.

should this break after setting result?  or, do you intend to take the last
matching result?  if so, then perhaps you should search from back to front
instead?

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