Bug 143880 - Favicons are not always loaded.
Summary: Favicons are not always loaded.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-17 06:45 PDT by peavo
Modified: 2015-04-20 09:52 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.54 KB, patch)
2015-04-17 06:49 PDT, peavo
no flags Details | Formatted Diff | Diff
Patch (2.32 KB, patch)
2015-04-17 06:51 PDT, peavo
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-mavericks (522.81 KB, application/zip)
2015-04-17 07:25 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (585.81 KB, application/zip)
2015-04-17 07:30 PDT, Build Bot
no flags Details
Patch (5.91 KB, patch)
2015-04-20 07:12 PDT, peavo
no flags Details | Formatted Diff | Diff
Patch (5.69 KB, patch)
2015-04-20 07:49 PDT, peavo
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 1 peavo 2015-04-17 06:49:21 PDT
Created attachment 251019 [details]
Patch
Comment 2 peavo 2015-04-17 06:51:58 PDT
Created attachment 251020 [details]
Patch
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[0];

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 8 peavo 2015-04-20 07:12:18 PDT
Created attachment 251158 [details]
Patch
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).
Comment 10 peavo 2015-04-20 07:49:54 PDT
Created attachment 251159 [details]
Patch
Comment 11 peavo 2015-04-20 09:52:32 PDT
Committed r183015: <http://trac.webkit.org/changeset/183015>