Summary: | Support for multiple <link rel="icon"> favicon elements. | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Rachel Blum <groby> | ||||||||||||||||||
Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | aa, abarth, ap, aroben, darin, dglazkov, fishd, koivisto, webkit.review.bot | ||||||||||||||||||
Priority: | P2 | Keywords: | HTML5 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||
OS: | All | ||||||||||||||||||||
Bug Depends on: | 68861 | ||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||
Attachments: |
|
Description
Rachel Blum
2011-08-02 13:16:05 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? * for each type of icon it encounters 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. Created attachment 103559 [details]
Patch
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. Created attachment 103563 [details]
Patch
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? +fishd; Thoughts on who would be the best person to review this patch? 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. 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? 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. (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. 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. > 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. > 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) (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)? 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) (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?) (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. Created attachment 105690 [details]
Patch - Support for multiple <link rel="icon"> items according to the API suggestion posted previously
Created attachment 106087 [details]
Patch
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
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.
Created attachment 106088 [details]
Patch
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? Created attachment 107083 [details]
Patch
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 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? Created attachment 107736 [details]
Patch
(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. Created attachment 108070 [details]
Patch
Comment on attachment 108070 [details] Patch Clearing flags on attachment: 108070 Committed r95593: <http://trac.webkit.org/changeset/95593> All reviewed patches have been landed. Closing bug. 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? (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 :-( |