Bug 143880 – Favicons are not always loaded.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

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.

Created attachment 251019 [details]
Patch

Created attachment 251020 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

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.

Created attachment 251158 [details]
Patch

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

Created attachment 251159 [details]
Patch

Committed r183015: <http://trac.webkit.org/changeset/183015>