Summary: | favicons should be saved to webarchives | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||
Component: | WebCore Misc. | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | aroben, beidson, darin | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Mac | ||||||||
OS: | OS X 10.5 | ||||||||
Attachments: |
|
Description
David Kilzer (:ddkilzer)
2008-12-10 13:38:31 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 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. |