Bug 65564 – Support for multiple <link rel="icon"> favicon elements.

• Component: DOM
• Status: RESOLVED FIXED

[Rachel Blum]

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.

[Adam Barth]

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?

[Adam Barth]

* for each type of icon it encounters

[Rachel Blum]

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.

[Rachel Blum]

Created attachment 103559 [details]
Patch

[Adam Barth]

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.

[Rachel Blum]

Created attachment 103563 [details]
Patch

[Adam Barth]

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?

[Adam Barth]

+fishd; Thoughts on who would be the best person to review this patch?

[Darin Fisher (:fishd, Google)]

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.

[Darin Fisher (:fishd, Google)]

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?

[Rachel Blum]

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.

[Darin Fisher (:fishd, Google)]

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

[Rachel Blum]

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.

[Darin Fisher (:fishd, Google)]

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

[Rachel Blum]

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

[Darin Fisher (:fishd, Google)]

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

[Rachel Blum]

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)

[Darin Fisher (:fishd, Google)]

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

[Rachel Blum]

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

[Rachel Blum]

Created attachment 105690 [details]
Patch - Support for multiple <link rel="icon"> items according to the API suggestion posted previously

[Rachel Blum]

Created attachment 106087 [details]
Patch

[Rachel Blum]

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

[WebKit Review Bot]

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.

[Rachel Blum]

Created attachment 106088 [details]
Patch

[Darin Fisher (:fishd, Google)]

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?

[Rachel Blum]

Created attachment 107083 [details]
Patch

[Rachel Blum]

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.

[Darin Fisher (:fishd, Google)]

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?

[Rachel Blum]

Created attachment 107736 [details]
Patch

[Rachel Blum]

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

[Rachel Blum]

Created attachment 108070 [details]
Patch

[WebKit Review Bot]

Comment on attachment 108070 [details]
Patch

Clearing flags on attachment: 108070

Committed r95593: <http://trac.webkit.org/changeset/95593>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Roben (:aroben)]

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?

[Darin Fisher (:fishd, Google)]

(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 :-(