Bug 112080 – FrameLoader::didChangeIcons isn't called when the favicon is changed.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[David Levin]

In Document::addIconURL, the check against iconURL fails.
        IconURL iconURL = f->loader()->icon()->iconURL(iconType);
        if (iconURL == newURL)

This is because IconController::iconURL as of http://trac.webkit.org/changeset/122806 returns the least favored favicon as opposed to the most favored.

A simple fix is the change the if in IconController::iconURL from

  if (result.m_iconURL.isEmpty() || !iter->m_mimeType.isEmpty())

to
  if (result.m_iconURL.isEmpty() || (result->m_mimeType.isEmpty() && !iter->m_mimeType.isEmpty()) {

so that the result is only set if it is empty or if it can be updated to have a mimeType when it doesn't already have one.

Related: As of r122806, caching m_iconURLs doesn't really do anything, so we should consider removing it (but that may take more work because it is returned by reference at the moment).

[David Levin]

It turns out that it is hard for webkit to determine what favicon will be used by the host so the filtering "if (iconURL == newURL)" ideally wouldn't be done at all and the host would have to figure out if the event is relevant or not.

Of course, it may not be ideal to send a bunch of events if there is a batch of favicon being processed so that should likely be coalesced into one notification.

[David Levin]

Created attachment 193928 [details]
Patch

[Build Bot]

Comment on attachment 193928 [details]
Patch

Attachment 193928 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17252116

[David Levin]

Created attachment 193951 [details]
Patch

[WebKit Review Bot]

Comment on attachment 193951 [details]
Patch

Clearing flags on attachment: 193951

Committed r146284: <http://trac.webkit.org/changeset/146284>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.