Bug 112080 - FrameLoader::didChangeIcons isn't called when the favicon is changed.
Summary: FrameLoader::didChangeIcons isn't called when the favicon is changed.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Levin
URL:
Keywords:
Depends on: 112614 112647
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-11 16:55 PDT by David Levin
Modified: 2013-03-19 18:12 PDT (History)
9 users (show)

See Also:


Attachments
Patch (4.99 KB, patch)
2013-03-19 14:46 PDT, David Levin
no flags Details | Formatted Diff | Diff
Patch (5.20 KB, patch)
2013-03-19 16:54 PDT, David Levin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2013-03-11 16:55:42 PDT
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).
Comment 1 David Levin 2013-03-13 13:34:56 PDT
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.
Comment 2 David Levin 2013-03-19 14:46:46 PDT
Created attachment 193928 [details]
Patch
Comment 3 Build Bot 2013-03-19 15:56:01 PDT
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
Comment 4 David Levin 2013-03-19 16:54:09 PDT
Created attachment 193951 [details]
Patch
Comment 5 WebKit Review Bot 2013-03-19 18:12:00 PDT
Comment on attachment 193951 [details]
Patch

Clearing flags on attachment: 193951

Committed r146284: <http://trac.webkit.org/changeset/146284>
Comment 6 WebKit Review Bot 2013-03-19 18:12:03 PDT
All reviewed patches have been landed.  Closing bug.