<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - Favicons are not always loaded."
href="https://bugs.webkit.org/show_bug.cgi?id=143880">bug 143880</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #251020 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Favicons are not always loaded."
href="https://bugs.webkit.org/show_bug.cgi?id=143880#c7">Comment # 7</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Favicons are not always loaded."
href="https://bugs.webkit.org/show_bug.cgi?id=143880">bug 143880</a>
from <span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=251020&action=diff" name="attach_251020" title="Patch">attachment 251020</a> <a href="attachment.cgi?id=251020&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=251020&action=review">https://bugs.webkit.org/attachment.cgi?id=251020&action=review</a>
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
<span class="quote">> Source/WebCore/loader/icon/IconController.cpp:-105
> - for (auto& candidate : iconsFromLinkElements(m_frame, LinkElementSelector::WithMIMEType))</span >
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.
<span class="quote">> Source/WebCore/loader/icon/IconController.cpp:103
> + auto icons = iconsFromLinkElements(m_frame, LinkElementSelector::All);
> + if (!icons.isEmpty())
> + return icons[0];</span >
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.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>