|Summary:||Favicons are not always loaded.|
|Component:||WebCore Misc.||Assignee:||Nobody <webkit-unassigned>|
|Severity:||Normal||CC:||buildbot, commit-queue, darin, japhet, rniwa|
|Version:||528+ (Nightly build)|
Description peavo 2015-04-17 06:45:53 PDT
If the favicon link element(s) in the document does not have a mime type, the favicon is loaded from the domain root (/favicon.ico). If no favicon exists at this location, the favicon loading will fail. This can be solved by not demanding that the link element has a mime type.
Comment 3 Build Bot 2015-04-17 07:25:12 PDT
Comment on attachment 251020 [details] Patch Attachment 251020 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5493359934177280 New failing tests: fast/dom/icon-url-list.html
Comment 4 Build Bot 2015-04-17 07:25:16 PDT
Created attachment 251021 [details] Archive of layout-test-results from ews102 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment 5 Build Bot 2015-04-17 07:30:46 PDT
Comment on attachment 251020 [details] Patch Attachment 251020 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6031344115122176 New failing tests: fast/dom/icon-url-list.html
Comment 6 Build Bot 2015-04-17 07:30:49 PDT
Created attachment 251022 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment 7 Darin Adler 2015-04-17 09:07:11 PDT
Comment on attachment 251020 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251020&action=review Seems like a good change. I believe this patch changes behavior in two ways: - It ignores MIME types entirely. - It chooses the last icon seen rather than the first one with the appropriate MIME type. We have to be sure that both of those changes are desired. I have a few thoughts: 1) First, a question: Is this fixing a regression from the recent changes, or has it behaved this way for a long time and we just noticed that it’s not good? 2) If we are going to change the behavior here, we should ask someone on the Safari team what effect this will have on Safari’s behavior. I’m not sure exactly how this is used there. It’s likely to make things better or have no effect, but I’d like to be sure about that before landing. Would be nice to check any other clients as well, but checking with the Safari folks seems particularly important to me. 3) If we are going to change the behavior here, we will need to change fast/dom/icon-url-list.html or its expected results so that it expects the behavior we now have. 4) I’d like to see enough test coverage so that both of the behavior changes here are actually reflected in tests. review- because the patch needs to at least revise the existing tests to pass, but also the tests need to show the change from less desirable behavior to better behavior > Source/WebCore/loader/icon/IconController.cpp:-105 > - for (auto& candidate : iconsFromLinkElements(m_frame, LinkElementSelector::WithMIMEType)) Since we are removing use of the "WithMIMEType" option, then we can, and should, remove the LinkElementSelector type entirely. I only added it so I could preserve this strange algorithm when refactoring the code. Lets not keep it. > Source/WebCore/loader/icon/IconController.cpp:103 > + auto icons = iconsFromLinkElements(m_frame, LinkElementSelector::All); > + if (!icons.isEmpty()) > + return icons; It seems that we are no longer using the vector of icons, just building it and ignoring all but the first icon in the vector. If so, then we ought to refactor the helper function so it no longer bothers to build a vector. That should be really easy to do.
Comment 9 peavo 2015-04-20 07:18:04 PDT
(In reply to comment #7) > Comment on attachment 251020 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=251020&action=review > Thanks for the review :) > Seems like a good change. > > I believe this patch changes behavior in two ways: > > - It ignores MIME types entirely. > - It chooses the last icon seen rather than the first one with the > appropriate MIME type. > > We have to be sure that both of those changes are desired. > The latest patch is consistent with previous behavior. If you think we should go on and ignore MIME types entirely and always pick the last icon, this is just a minor modification of the current patch. > I have a few thoughts: > > 1) First, a question: Is this fixing a regression from the recent changes, > or has it behaved this way for a long time and we just noticed that it’s not > good? I believe this is a recent regression, where favicons without mime type are not loaded (unless there is a favicon at the default location).