* SUMMARY When a webarchive of a page is saved, the favicon is never saved with the webarchive. It does not matter whether the favicon is specified in a <link rel="icon"> tag or whether it's at the default path (/favicon.ico) of the web server. * STEPS TO REPRODUCE 1. Navigate to a web site with a favicon. Example: <http://digg.com/> 2. Save the page as a webarchive. 3. Convert the webarchive to "xml1" format using plutil. 4. Open the text-based webarchive in a text editor. 5. Search for "favicon" or the URL of the <link rel="icon"> tag. * RESULTS None of the saved resources in the webarchive include the favicon. * REGRESSION This is not a regression. I don't believe the favicon was ever saved. * NOTES Because the favicon is loaded by completely different code than the rest of the resources on a web page, a special case must be added for it when saving a webarchive.
Created attachment 25925 [details] Patch v1 Proposed patch. A couple of notes: - Should I expose a method in WebIconDatabasePrivate.h to disable the icon database instead of using a WebIconDatabaseInternal method? Or should I add _applicationWillTerminate: to WebIconDatabaseInternal.h and make the header Private so it is copied to PrivateHeaders? - Similarly for the Windows LayoutTestController::setIconDatabaseEnabled(bool) method, should I expose a WebIconDatabase::close() method instead of calling shutDownWebKit()? - I have not tried to compile the Windows code yet.
Comment on attachment 25925 [details] Patch v1 > + RefPtr<SharedBuffer> data = iconDatabase()->iconForPageURL(responseURL, IntSize(16, 16))->data(); > + RefPtr<ArchiveResource> resource = ArchiveResource::create(data, KURL(iconURL), "image/x-icon", "", ""); This should be data.release(). r=me
Darin didn't address any of my comments in Comment #1, and I don't have a Windows development environment handy, so it may be a while before I can land this.
Comment on attachment 25925 [details] Patch v1 > Has mac implementation :-(
Comment on attachment 25925 [details] Patch v1 Clearing Darin's review+ flag since I need to address the issues in Comment #1 using WebIconDatabase SPI.
(In reply to comment #4) > (From update of attachment 25925 [details] [review]) > > Has mac implementation > > :-( Sounds like you're volunteering to fix: <rdar://problem/6436020> Implement WebArchive test support for DumpRenderTree on Windows :-)
Note: This patch should also add a method to the icon database to return the original image (unmodified) for saving in the WebArchive.
(In reply to comment #7) > Note: This patch should also add a method to the icon database to return the > original image (unmodified) for saving in the WebArchive. For now IconRecord::image(const IntSize&) actually ignores the size passed in, so this isn't necessary.
Created attachment 26519 [details] Patch v3 (there is no Patch v2) Patch v2 was an internal patch which never saw the light of day. Changes since Patch v1: - Fixed data.release() issue from Comment #2. - Fixed bug in IconDatabase::close() so that the method actually closes the internal SQLite database. This makes isOpen() report the correct state immediately afterward calling close(). - Worked around <rdar://problem/6480108> in LayoutTestControllerMac.mm and LayoutTestControllerWin.cpp. (Basically, [[WebIconDatabase sharedIconDatabase] setEnabled:FALSE] doesn't work because getting the shared instance for the first time enables the database, and disabling it right away doesn't work. Instead, DumpRenderTree assumes it's the only client using the WebIconDatabase, and that the icon database is disabled until [WebIconDatabase sharedIconDatabase] is called.)
Comment on attachment 26519 [details] Patch v3 (there is no Patch v2) r=me But *should* favicons really be saved in web archives?
(In reply to comment #10) > (From update of attachment 26519 [details] [review]) > r=me > > But *should* favicons really be saved in web archives? For faithful reproduction (or archive) of a web page, I think they should. Note that this method is only used (AFAICT) to create webarchive files, not for saving a DOM to the pasteboard.
$ git svn dcommit Committing to http://svn.webkit.org/repository/webkit/trunk ... M LayoutTests/ChangeLog A LayoutTests/webarchive/resources/favicon.ico A LayoutTests/webarchive/test-link-rel-icon-expected.webarchive A LayoutTests/webarchive/test-link-rel-icon.html M WebCore/ChangeLog M WebCore/html/HTMLLinkElement.cpp M WebCore/loader/archive/cf/LegacyWebArchive.cpp M WebCore/loader/icon/IconDatabase.cpp M WebKitTools/ChangeLog M WebKitTools/DumpRenderTree/LayoutTestController.cpp M WebKitTools/DumpRenderTree/LayoutTestController.h M WebKitTools/DumpRenderTree/mac/DumpRenderTree.mm M WebKitTools/DumpRenderTree/mac/LayoutTestControllerMac.mm M WebKitTools/DumpRenderTree/win/DumpRenderTree.cpp M WebKitTools/DumpRenderTree/win/LayoutTestControllerWin.cpp Committed r39904
Follow-up build fix for GTK port: $ git svn dcommit Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebKitTools/ChangeLog M WebKitTools/DumpRenderTree/gtk/DumpRenderTree.cpp M WebKitTools/DumpRenderTree/gtk/LayoutTestControllerGtk.cpp Committed r39906
Follow-up fix for Windows: $ git svn dcommit Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebKitTools/ChangeLog M WebKitTools/DumpRenderTree/win/LayoutTestControllerWin.cpp Committed r39914 http://trac.webkit.org/changeset/39914
Note that the layout test in this bug was disabled later. See Bug 23331.