Bug 22795 - favicons should be saved to webarchives
Summary: favicons should be saved to webarchives
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-12-10 13:38 PST by David Kilzer (:ddkilzer)
Modified: 2009-01-28 19:50 PST (History)
3 users (show)

See Also:


Attachments
Patch v1 (21.27 KB, patch)
2008-12-10 13:55 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v3 (there is no Patch v2) (24.20 KB, patch)
2009-01-07 18:50 PST, David Kilzer (:ddkilzer)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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
Comment 15 David Kilzer (:ddkilzer) 2009-01-28 19:50:29 PST
Note that the layout test in this bug was disabled later.  See Bug 23331.