Bug 65564 - Support for multiple <link rel="icon"> favicon elements.
Summary: Support for multiple <link rel="icon"> favicon elements.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: HTML5
Depends on: 68861
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-02 13:16 PDT by Rachel Blum
Modified: 2011-09-26 18:25 PDT (History)
9 users (show)

See Also:


Attachments
Patch (19.50 KB, patch)
2011-08-10 17:03 PDT, Rachel Blum
no flags Details | Formatted Diff | Diff
Patch (19.56 KB, patch)
2011-08-10 17:38 PDT, Rachel Blum
no flags Details | Formatted Diff | Diff
Patch - Support for multiple <link rel="icon"> items according to the API suggestion posted previously (11.82 KB, patch)
2011-08-30 14:12 PDT, Rachel Blum
no flags Details | Formatted Diff | Diff
Patch (19.32 KB, patch)
2011-09-01 20:17 PDT, Rachel Blum
no flags Details | Formatted Diff | Diff
Patch (19.32 KB, patch)
2011-09-01 20:22 PDT, Rachel Blum
no flags Details | Formatted Diff | Diff
Patch (19.30 KB, patch)
2011-09-12 14:22 PDT, Rachel Blum
no flags Details | Formatted Diff | Diff
Patch (19.34 KB, patch)
2011-09-16 15:42 PDT, Rachel Blum
no flags Details | Formatted Diff | Diff
Patch (19.34 KB, patch)
2011-09-20 16:10 PDT, Rachel Blum
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rachel Blum 2011-08-02 13:16:05 PDT
HTML5 supports the idea for multiple favicons - http://www.whatwg.org/specs/web-apps/current-work/multipage/links.html#rel-icon - but WebKit currently only supports three different specific icons (FavIcon, TouchIcon, TouchPrecomposedIcon). Chromium will shortly need that support. (Larger bookmark icons). Bug #37674 was resolved in preparation to this and contains additional info.

I can see two possible ways to implement this:

1) As described in Bug #37674

* collect al* rel-icon links when WebKit does the initial tree traversal - LinkLoader::loadLink is called for each <link rel="icon"> entry.
* WebCore::Document will be extended to contain a list of all icons encountered. (Currently, only keeps track of three predefined ones). Document::setIconURL will track them, and they will be exposed via accessor

2) link element callback on WebCore::Chrome, invoked by HTMLLinkElement constructor

Both methods are backwards-compatible. Existing APIs are not changed, FavIcon selection is not modified, no additional resources will be loaded.

#1 is consistent with the current approach to icons, but requires more code in WebCore. #2 makes it very clear that FavIcon selection is a job outside of WebCore, and has a smaller code footprint.
Comment 1 Adam Barth 2011-08-09 13:13:49 PDT
I haven't looked at this code in much detail, but the way I would expect this to work is for WebCore to call back into the client to ask for each type of icon whether the client is interested in loading it.  If so, it kicks of the load and tells the client when the icon is available.  I guess that's more like (2) above?
Comment 2 Adam Barth 2011-08-09 13:14:13 PDT
* for each type of icon it encounters
Comment 3 Rachel Blum 2011-08-10 16:55:49 PDT
Actually not touching the way icons are loaded (yet) - WebCore::Document has a very specific way of handling icons, and web sites rely on quirks in that. The patch (to follow soon) will simply implement a callback mechanism on ChromeClient - it is up to projects using WebKit to take advantage (or not) of this.
Comment 4 Rachel Blum 2011-08-10 17:03:27 PDT
Created attachment 103559 [details]
Patch
Comment 5 Adam Barth 2011-08-10 17:05:49 PDT
Comment on attachment 103559 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103559&action=review

> Source/WebCore/html/HTMLLinkElement.cpp:241
> +      document()->page()->chrome()->registerFaviconSource(
> +          getAttribute(hrefAttr), getAttribute(sizesAttr), getAttribute(typeAttr));

Bad indent.  What if page is null?  getAttribute -> fastGetAttribute

> Source/WebKit/chromium/public/WebViewClient.h:319
> +    virtual void registerFaviconSource(const WebString& url,
> +                                        const WebString& sizes,
> +                                        const WebString& mediaType) { }

Bad indent.
Comment 6 Rachel Blum 2011-08-10 17:38:20 PDT
Created attachment 103563 [details]
Patch
Comment 7 Adam Barth 2011-08-12 13:16:50 PDT
Comment on attachment 103563 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103563&action=review

I don't really understand this patch well enough to review it.  I'm not sure who would be a better reviewer either.  :-/  fishd might have some suggestions.

> Source/WebKit/chromium/public/WebViewClient.h:316
> +    // Registers <link rel="icon"> elements.

What does that mean?
Comment 8 Adam Barth 2011-08-12 13:17:15 PDT
+fishd; Thoughts on who would be the best person to review this patch?
Comment 9 Darin Fisher (:fishd, Google) 2011-08-15 14:12:52 PDT
Comment on attachment 103563 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103563&action=review

> Source/WebCore/html/HTMLLinkElement.cpp:240
> +        document()->page()->chrome()->registerFaviconSource(

I think other ports use an icon database that is built into WebCore.  How does this work with those ports?
See WebCore/loader/icon/IconDatabase.{h,cpp}

>> Source/WebKit/chromium/public/WebViewClient.h:316
>> +    // Registers <link rel="icon"> elements.
> 
> What does that mean?

I have the same question as Adam.
Comment 10 Darin Fisher (:fishd, Google) 2011-08-15 14:14:30 PDT
Also, how does this relate to WebFrame::iconURLs and WebFrameClient::didChangeIcon?  The iconURLs method gives the embedder a way to query the set of available icons, and didChangeIcon gives WebKit a way to tell the embedder when the set of available icons changes.  Why do we need registerFaviconSource?
Comment 11 Rachel Blum 2011-08-15 14:21:11 PDT
Re: // Registers <link rel="icon"> elements

This is a callback for any favicon links encountered, so clients can handle icons separately from the core mechanism in WebKit.

Which dovetails with the question re: IconDatabase, WebFrame::iconURLs, WebFrameClient::didChangeIcon. These continue behaving _exactly_ as before, since :darin in https://bugs.webkit.org/show_bug.cgi?id=37674#c1 stresses the importance of backwards compatibility, and :abarth in https://bugs.webkit.org/show_bug.cgi?id=65564#c1 suggested the callback method in favor of changing LinkLoader::loadLink and the iconDB

If you think this is the wrong direction to head, I'd appreciate your feedback on how to proceed.
Comment 12 Darin Fisher (:fishd, Google) 2011-08-15 16:34:18 PDT
(In reply to comment #11)
> If you think this is the wrong direction to head, I'd appreciate your feedback on how to proceed.

Can you explain how this is not redundant with WebFrame::iconURLs and WebFrameClient::didChangeIcon?  Those are about revealing the set of available favicons.  The summary of this bug report is support for multiple favicons.  Sounds like exactly the same thing.
Comment 13 Rachel Blum 2011-08-15 16:41:23 PDT
IconController::urlsForTypes is hard-coded to support exactly three icon types: FavIcon, TouchIcon, TouchPrecomposedIcon. It does not (even though it looks that way) support multiple icons beyond "one of each" for those three. 

The patch in bug #37674 originally added support for a fourth type, "AnyIcon", which could be used to return a list of all icons. I split that out to keep sizes support separate from multi-icon support, and based on comment #1 here I picked a different approach. That might well have been wrong :)

If you think the original approach in #37674 would've been better (it's the second patch, if you want to take a look), I'm happy to resurrect that.
Comment 14 Darin Fisher (:fishd, Google) 2011-08-15 17:06:04 PDT
> IconController::urlsForTypes is hard-coded to support exactly three icon types: FavIcon, TouchIcon, TouchPrecomposedIcon. It does not (even though it looks that way) support multiple icons beyond "one of each" for those three. 

I would expect it to be able to return multiple entries for each type.  The first entry of any given type should probably be the preferred, default icon.


> The patch in bug #37674 originally added support for a fourth type, "AnyIcon", which could be used to return a list of all icons. I split that out to keep sizes support separate from multi-icon support, and based on comment #1 here I picked a different approach. That might well have been wrong :)

Comment #1 doesn't seem to take into account the fact that favicon loading in Chromium is done by Chromium and not WebKit.  Chromium just gets the URL of the favicon, and then Chromium loads it externally to WebKit.  (We use webkit_glue::ImageResourceFetcher.)  It might make sense for WebKit to fetch it, but in reality, the favicons are just needed by the browser UI.  That's why we pushed all of the loading responsibility onto the embedder of WebKit.


> If you think the original approach in #37674 would've been better (it's the second patch, if you want to take a look), I'm happy to resurrect that.

I'm not sure.  That patch seems to have a lot to do with tracking icon sizes too.  I need to study it further to answer your question well.

It just seems fairly confusing to add a redundant sounding API for getting favicon information.
Comment 15 Rachel Blum 2011-08-15 17:37:10 PDT
> I would expect it to be able to return multiple entries for each type.  The first entry of any given type should probably be the preferred, default icon.

That won't work, unfortunately - the icon type is a bitmask (so you can e.g. get both FavIcon and TouchIcon in one go). That means a varying number of icons are default icons, followed by non-default ones. Since that's an API change, it might break other WebKit users.

I don't disagree that a second API is confounding - but in light of the fact that the old API is brittle *and* that we need backwards compatibility, it seemed best to pick an API that is completely invisible to clients relying on the old API.

> need to study it further to answer your question well

I'll try to give a summary:

Document::setIconURL is extended to keep a list of all icons encountered, in addition to the 3 "default" ones it tracks. (The number of default icons is hard-coded to ICON_COUNT)

IconController::urlsForTypes returns that list of all icons if you pass in AnyIcon. Otherwise, it keeps the old behavior. Combining AnyIcon with the other types as operation does not really make sense, but it probably should result in the same behavior as if only AnyIcon was passed in.

IconController::appendToIconURLs is modified to retrieve the list of all icons kept on the Document if you pass in setIconURL

---- End of summary ---

You are right, there is more code in that patch to actually pass lists of sizes along with IconURLs. That's completely orthogonal to the changes above, so you can probably ignore it. (And if you think the above approach is better, I'll probably try to land that first, and sizes support in a second bug)
Comment 16 Darin Fisher (:fishd, Google) 2011-08-18 14:18:50 PDT
(In reply to comment #15)
> > I would expect it to be able to return multiple entries for each type.  The first entry of any given type should probably be the preferred, default icon.
> 
> That won't work, unfortunately - the icon type is a bitmask (so you can e.g. get both FavIcon and TouchIcon in one go). That means a varying number of icons are default icons, followed by non-default ones. Since that's an API change, it might break other WebKit users.
> 
> I don't disagree that a second API is confounding - but in light of the fact that the old API is brittle *and* that we need backwards compatibility, it seemed best to pick an API that is completely invisible to clients relying on the old API.

Why is backwards compat an issue?  We are constantly evolving the WebKit API.  We only need backwards compat temporarily to support WebKit rolls.  We often invent the better API and deprecate the older API, but in this case, it sounds like you are proposing a new API that does not deprecate the old API.  That means a future that is more complex, which makes me sad.

Why do we need this sizes attribute at all?  An .ico file is basically a mipmap, containing multiple images at different resolutions.  Are you trying to use non-ico icons (e.g., PNGs)?
Comment 17 Rachel Blum 2011-08-24 14:36:23 PDT
Proposal for a patch with minimal effect on the current API: https://docs.google.com/document/pub?id=1FbxGOXcwEKIXsh44NESRpgHXYIt1MnCUoqgueZ-_U6Y

The summary: API's stay mostly the same. Signatures get changed to accomodate sizes where appropriate, and one function changes from returning an IconURL to returning Vector<IconURL> - smallest possible change.

We do need sizes since the HTML5 spec explicitly allows for them. (And with larger icons, they allow only downloading icons of appropriate size without peeking into every file)
Comment 18 Darin Fisher (:fishd, Google) 2011-08-29 12:36:16 PDT
(In reply to comment #17)
> Proposal for a patch with minimal effect on the current API: https://docs.google.com/document/pub?id=1FbxGOXcwEKIXsh44NESRpgHXYIt1MnCUoqgueZ-_U6Y
> 
> The summary: API's stay mostly the same. Signatures get changed to accomodate sizes where appropriate, and one function changes from returning an IconURL to returning Vector<IconURL> - smallest possible change.
> 
> We do need sizes since the HTML5 spec explicitly allows for them. (And with larger icons, they allow only downloading icons of appropriate size without peeking into every file)

Some comments:

 - Overall, sounds like a good plan.

 - Document::setIconURL should probably be changed to addIconURL since there could be multiple icon URLs, right?

 - Your document doesn't indicate how the Chromium WebKit API will be impacted.

 - I tend to agree with you that making Document::iconURL return a Vector is better than introducing a separate allIconURLs function.  (shouldn't iconURL be named iconURLs?)
Comment 19 Rachel Blum 2011-08-29 13:45:57 PDT
(In reply to comment #18)
>  - Overall, sounds like a good plan.

I'll start getting the patch ready, then.

>  - Document::setIconURL should probably be changed to addIconURL since there could be multiple icon URLs, right?

Yes and no. For the pre-defined types of icon (FavIcon, TouchIcon, TouchIconPrecomputed) this will keep existing behavior and just set one icon. I've got no preference either way, since neither name catches the meaning 100%. 

I'll go with the rename for now.

>  - Your document doesn't indicate how the Chromium WebKit API will be impacted.

It isn't :)

Chromium still gets notified via ChromeRenderViewObserver::DidIconChange(), and it obtains a list of icons directly from the IconController.

>  - I tend to agree with you that making Document::iconURL return a Vector is better than introducing a separate allIconURLs function.  (shouldn't iconURL be named iconURLs?)

It probably should. Changing the name.
Comment 20 Rachel Blum 2011-08-30 14:12:27 PDT
Created attachment 105690 [details]
Patch - Support for multiple <link rel="icon"> items according to the API suggestion posted previously
Comment 21 Rachel Blum 2011-09-01 20:17:15 PDT
Created attachment 106087 [details]
Patch
Comment 22 Rachel Blum 2011-09-01 20:19:15 PDT
Comment on attachment 106087 [details]
Patch

Added requested refactoring:

* DocumentLoader now is not involved in icons at all, no storage of icons either
* Document simply stores a list of icons, without any logic. Notification didChangeIcon is triggered from Document now.
* IconController handles the logic of retrieving the preferred icon for a specific type
Comment 23 WebKit Review Bot 2011-09-01 20:20:39 PDT
Attachment 106087 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:20:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:21:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 2 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 Rachel Blum 2011-09-01 20:22:57 PDT
Created attachment 106088 [details]
Patch
Comment 25 Darin Fisher (:fishd, Google) 2011-09-07 15:00:24 PDT
Comment on attachment 106088 [details]
Patch

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?
Comment 26 Rachel Blum 2011-09-12 14:22:52 PDT
Created attachment 107083 [details]
Patch
Comment 27 Rachel Blum 2011-09-12 14:25:44 PDT
Comment on attachment 106088 [details]
Patch

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.
Comment 28 Darin Fisher (:fishd, Google) 2011-09-16 14:46:57 PDT
Comment on attachment 107083 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107083&action=review

> Source/WebCore/dom/Document.cpp:4372
> +      IconURL favIconURL = f->loader()->icon()->iconURL(iconType);

nit: indentation should be 4 white spaces

> Source/WebCore/dom/Document.cpp:4373
> +      if (favIconURL == newURL)

why do you need to restrict calls to didChangeIcons?  also, why is this variable
named favIconURL?  couldn't iconType indicate a different type of icon?

> Source/WebCore/dom/IconURL.h:59
>          : m_iconType(InvalidIcon)

this should initialize m_isDefaultIcon

> Source/WebCore/dom/IconURL.h:72
> +    static IconURL DefaultIconURL(const KURL&, IconType);

nit: method names start with a lower case letter

> Source/WebCore/loader/icon/IconController.cpp:120
> +            iconURLs.append(*iter);

should you be increasing iconCount here since iconURLs.size() is increasing?
Comment 29 Rachel Blum 2011-09-16 15:42:11 PDT
Created attachment 107736 [details]
Patch
Comment 30 Rachel Blum 2011-09-16 15:45:12 PDT
(In reply to comment #28)

Fixed review issues, except

> why do you need to restrict calls to didChangeIcons?  also, why is this variable
> named favIconURL?  couldn't iconType indicate a different type of icon?

It's named favIconURL since it only notifies if the FavIcon of the particular icon type (FavIcon, Touch, PrecomputedTouch) has changed. Document _used_ to keep only one icon of each type and notified when that changed - this patch stores all icons, but still only notifies if the one that will be selected as the FavIcon by the browser changed.
Comment 31 Rachel Blum 2011-09-20 16:10:29 PDT
Created attachment 108070 [details]
Patch
Comment 32 WebKit Review Bot 2011-09-20 18:48:12 PDT
Comment on attachment 108070 [details]
Patch

Clearing flags on attachment: 108070

Committed r95593: <http://trac.webkit.org/changeset/95593>
Comment 33 WebKit Review Bot 2011-09-20 18:48:19 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 Adam Roben (:aroben) 2011-09-21 07:01:21 PDT
Comment on attachment 108070 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108070&action=review

> Source/WebCore/ChangeLog:8
> +        No tests - purely an API change. (And API is not exposed to LayoutTests)

Presumably this could be tested via window.internals?
Comment 35 Darin Fisher (:fishd, Google) 2011-09-21 09:15:25 PDT
(In reply to comment #34)
> (From update of attachment 108070 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108070&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        No tests - purely an API change. (And API is not exposed to LayoutTests)
> 
> Presumably this could be tested via window.internals?

Oh, that's a really good suggestion!  I'm not used to thinking about window.internals :-(