[Webkit-unassigned] [Bug 143880] Favicons are not always loaded.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Apr 17 09:07:11 PDT 2015


https://bugs.webkit.org/show_bug.cgi?id=143880

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #251020|review?                     |review-
              Flags|                            |

--- Comment #7 from Darin Adler <darin at apple.com> ---
Comment on attachment 251020
  --> https://bugs.webkit.org/attachment.cgi?id=251020
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150417/748e6a88/attachment.html>


More information about the webkit-unassigned mailing list