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

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


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


Rachel Blum <groby at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #106088|                            |review?, commit-queue?
               Flag|                            |




--- Comment #27 from Rachel Blum <groby at chromium.org>  2011-09-12 14:25:44 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.

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.

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



More information about the webkit-unassigned mailing list