[webkit-reviews] review denied: [Bug 65564] Support for multiple <link rel="icon"> favicon elements. : [Attachment 106088] 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 106088: Patch
https://bugs.webkit.org/attachment.cgi?id=106088&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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?