|Summary:||favicons should be saved to webarchives|
|Product:||WebKit||Reporter:||David Kilzer (:ddkilzer) <ddkilzer>|
|Component:||WebCore Misc.||Assignee:||David Kilzer (:ddkilzer) <ddkilzer>|
|Severity:||Normal||CC:||aroben, beidson, darin|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.5|
Description David Kilzer (:ddkilzer) 2008-12-10 13:38:31 PST
* 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.
Comment 1 David Kilzer (:ddkilzer) 2008-12-10 13:55:03 PST
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 2 Darin Adler 2008-12-10 14:25:54 PST
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
Comment 3 David Kilzer (:ddkilzer) 2008-12-10 14:37:14 PST
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 4 Mark Rowe (bdash) 2008-12-10 15:36:50 PST
Comment on attachment 25925 [details] Patch v1 > Has mac implementation :-(
Comment 5 David Kilzer (:ddkilzer) 2008-12-10 17:05:42 PST
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.
Comment 6 David Kilzer (:ddkilzer) 2008-12-10 17:06:51 PST
(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 :-)
Comment 7 David Kilzer (:ddkilzer) 2008-12-11 06:58:20 PST
Note: This patch should also add a method to the icon database to return the original image (unmodified) for saving in the WebArchive.
Comment 8 David Kilzer (:ddkilzer) 2008-12-22 21:18:15 PST
(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.
Comment 9 David Kilzer (:ddkilzer) 2009-01-07 18:50:18 PST
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 10 Darin Adler 2009-01-08 10:00:39 PST
Comment on attachment 26519 [details] Patch v3 (there is no Patch v2) r=me But *should* favicons really be saved in web archives?
Comment 11 David Kilzer (:ddkilzer) 2009-01-08 10:33:13 PST
(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.
Comment 12 David Kilzer (:ddkilzer) 2009-01-14 13:08:11 PST
$ 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
Comment 13 David Kilzer (:ddkilzer) 2009-01-14 13:24:27 PST
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
Comment 14 David Kilzer (:ddkilzer) 2009-01-14 15:19:43 PST
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