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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Sep 12 14:25:44 PDT 2011


Rachel Blum <groby at chromium.org> has asked  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 Rachel Blum <groby 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.

Added the DefaultIconURL factory method so isDefault is no longer parameter of
constructor

>> 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).

m_isDefault (now renamed to m_isDefaultIcon, for clarity) is set in defaultURL
in IconController. It's purely a signifier for higher level code (e.g.
Chromium) that this is a "default" icon that has been guessed by IconController
(i.e. "/favicon.ico"), not an icon specified by the user

>> 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?

We can't simply take the last matching result. The icon is the first icon we
find, unless later icons have a mime type, in which case it's the last icon
with a mime type we find. Since the number of icons on a page is usually small
& forward iter is slightly clearer than backwards iter, let's stick with
forward.


More information about the webkit-reviews mailing list