Bug 22795 – favicons should be saved to webarchives

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Kilzer (:ddkilzer)]

* 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.

[David Kilzer (:ddkilzer)]

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.

[Darin Adler]

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

[David Kilzer (:ddkilzer)]

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.

[Mark Rowe (bdash)]

Comment on attachment 25925 [details]
Patch v1

> Has mac implementation

:-(

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

(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

:-)

[David Kilzer (:ddkilzer)]

Note: This patch should also add a method to the icon database to return the original image (unmodified) for saving in the WebArchive.

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

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.)

[Darin Adler]

Comment on attachment 26519 [details]
Patch v3 (there is no Patch v2)

r=me

But *should* favicons really be saved in web archives?

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

$ 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

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

Note that the layout test in this bug was disabled later.  See Bug 23331.