[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