The nsIDownloadHistory interface should also store a moz-icon with the download. This means we should add an additional optional argument for the destination URI. We can do this in both consumers (GetTargetFile in both nsDownload and nsExternalAppHandler), and then we should asynchronously add the favicon via a moz-icon with the file uri. I just checked, and moz-icon:file:///usr/bin/local/test.png properly showed a png icon even on windows, so our code looks smart enough. Then we get icons for it in the library!

Comment on attachment 447116 [details] [diff] [review] Patch with test case. >+++ b/docshell/base/nsDownloadHistory.cpp >@@ -62,11 +62,11 @@ interface nsIDownloadHistory : nsISuppor > * [optional] The time the download was started. If the start time > * is not given, the current time is used. > * @throws NS_ERROR_NOT_AVAILABLE > * In a situation where a history implementation is not available, > * where 'history implementation' refers to something like > * nsIGlobalHistory and friends. > */ > void addDownload(in nsIURI aSource, [optional] in nsIURI aReferrer, >- [optional] in PRTime aStartTime); >+ [optional] in PRTime aStartTime, [optional] in nsIURI aTarget); You did not update the documentation. >+++ b/toolkit/components/downloads/src/nsDownloadManager.cpp >+ GetTargetFile(getter_AddRefs(targetFile)); This can fail, but we don't care. Cast to (void) please. >+ nsCOMPtr<nsIURI> target; >+ if (targetFile) >+ NS_NewFileURI(getter_AddRefs(target), targetFile); same with this >+++ b/toolkit/components/places/src/AsyncFaviconHelpers.cpp >+ // If possible, determine the MIME type from the request and use it as a failsafe. >+ nsCOMPtr<nsIChannel> channel(do_QueryInterface(aRequest)); >+ nsCAutoString contentType; >+ channel->GetContentType(contentType); >+ static nsCAutoString whitelist[] = >+ { (nsCAutoString) NS_LITERAL_CSTRING("image/icon"), >+ (nsCAutoString) NS_LITERAL_CSTRING("image/x-icon") }; >+ PRBool isInWhitelist = PR_FALSE; >+ for (PRUint32 i = 0; i < NS_ARRAY_LENGTH(whitelist); ++i) { >+ isInWhitelist |= whitelist[i].Equals(contentType); >+ } >+ if (!isInWhitelist) >+ contentType.Truncate(0); This logic should be pulled out into a helper method. >+++ b/toolkit/components/places/src/nsNavHistory.cpp >+ NS_NewURI(getter_AddRefs(target), NS_LITERAL_CSTRING("moz-icon:") + spec); This can fail, check it. >+ nsCOMPtr<nsIFaviconService> faviconService = >+ do_GetService(NS_FAVICONSERVICE_CONTRACTID); nit: only indent this one indentation level. r=sdwilsh with comments addressed.

Comment on attachment 447257 [details] [diff] [review] Fixed as per sdwilsh's comments. Already gave this r+, so I don't need to see it again.

Comment on attachment 447257 [details] [diff] [review] Fixed as per sdwilsh's comments. >diff --git a/docshell/base/nsIDownloadHistory.idl b/docshell/base/nsIDownloadHistory.idl you need SR on API changes >diff --git a/toolkit/components/downloads/src/nsDownloadManager.cpp b/toolkit/components/downloads/src/nsDownloadManager.cpp >diff --git a/toolkit/components/places/src/AsyncFaviconHelpers.cpp b/toolkit/components/places/src/AsyncFaviconHelpers.cpp >+/** >+ * Helper method for a content type whitelist. The whitelist is used to filter >+ * content types which can be used as an icon and which are not provided by >+ * GetMIMETypeFromContent. >+ */ >+static PRBool >+isContentTypeInWhitelist(nsCString& aContentType) >+{ >+ static nsCAutoString whitelist[] = >+ { (nsCAutoString) NS_LITERAL_CSTRING("image/icon"), >+ (nsCAutoString) NS_LITERAL_CSTRING("image/x-icon") }; I don't like how this is indented >+ for (PRUint32 i = 0; i < NS_ARRAY_LENGTH(whitelist); ++i) { >+ return PR_TRUE; >+ } >+ return PR_FALSE; >+} Am I crazy or this always returns true? there is no check in the loop Btw, due to the size of this, I'd prefer a simple if/else if >diff --git a/toolkit/components/places/tests/unit/test_download_favicons.js b/toolkit/components/places/tests/unit/test_download_favicons.js xpcshell tests in Places give you Services, PlacesUtils and NetUtil for free, so use them! (see http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/head_common.js) >+// Get services useless comment >+var hs = Cc["@mozilla.org/browser/nav-history-service;1"]. >+ getService(Ci.nsINavHistoryService); PlacesUtils.history >+var os = Cc["@mozilla.org/observer-service;1"]. >+ getService(Ci.nsIObserverService); Services.obs >+var prefs = Cc["@mozilla.org/preferences-service;1"]. >+ getService(Ci.nsIPrefBranch); Services.prefs >+var dh = Cc["@mozilla.org/browser/download-history;1"]. >+ getService(Ci.nsIDownloadHistory); we should add a lazygetter for this to PlacesUtils >+var icons = Cc["@mozilla.org/browser/favicon-service;1"]. >+ getService(Ci.nsIFaviconService); PlacesUtils.favicons >+ >+var testURI = uri("http://google.com/"); >+var referrerURI = uri("http://getfirefox.com"); these should be const and uppercase like const TEST_URI = ... >+ QueryInterface: function(iid) { >+ if (iid.equals(Ci.nsINavHistoryObserver) || >+ iid.equals(Ci.nsISupports)) { >+ return this; >+ } >+ throw Cr.NS_ERROR_NO_INTERFACE; >+ } XPCOMUtils.generateQI >+var currentIndex = 0; >+var tests = [ >+ { url: "file:///test.png" }, >+ { url: "file:///test.dmg" }, >+ { url: "file:///test.gif" }]; >+ >+// main >+function run_test() { >+ do_test_pending(); >+ hs.addObserver(observer, false); >+// dh.addDownload(testURI, referrerURI, Date.now() * 1000, >+// tests[currentIndex]); what is this commented part?? r- for the missing check in the whitelist and the test. I still think makes no sense to add these icons to the DB, because the user can change system icons associating the filetype to another application, for example I download a 7z file, the saved icon is white because I don't have 7-zip installed, I install it and open the file that now has the 7-zip icon. When I look at my download history the file has a blank icon still (and moreover we have saved a useless blank icon). I think we will get this bug filed as soon as FX4 goes gold. Btw as a temporary workaround I'll close both my eyes if you feel so confident about this fix.

(In reply to comment #5) > I still think makes no sense to add these icons to the DB, because the user can > change system icons associating the filetype to another application, for > example I download a 7z file, the saved icon is white because I don't have > 7-zip installed, I install it and open the file that now has the 7-zip icon. > When I look at my download history the file has a blank icon still (and > moreover we have saved a useless blank icon). I think we will get this bug > filed as soon as FX4 goes gold. I guess dietrich needs to make a call here since we have two peers in disagreement :/

I just wanted to express what are the actual problems this approach is not covering. We can easily kill of of these icons with PlacesDBUtils once we have the right fix in (that is having a moz-anno:icon protocol implementing pass-through for icons not in the db and allowing to have icon entries without data), should just be matter of removing all icons associated to a page that has a _DOWNLOAD visit. That's why I said I can close my eyes, if it would have been harder to remove this temporary fix I'd have said "NO NO NO".

Superreview requested for the interface change.

Interface change looks fine, only skimmed the rest; mak's review should cover that!

Comment on attachment 450393 [details] [diff] [review] Fixed another copy&paste fail and cleaned up test. >diff --git a/toolkit/components/places/src/AsyncFaviconHelpers.cpp b/toolkit/components/places/src/AsyncFaviconHelpers.cpp >+/** >+ * Helper method for a content type whitelist. The whitelist is used to filter >+ * content types which can be used as an icon and which are not provided by >+ * GetMIMETypeFromContent. >+ */ >+static PRBool >+isContentTypeInWhitelist(nsCString& aContentType) shouldn't this be a "const" nsCString&? >+{ >+ if (aContentType.Equals(NS_LITERAL_CSTRING("image/icon")) || >+ aContentType.Equals(NS_LITERAL_CSTRING("image/x-icon"))) >+ return PR_TRUE; You can use .EqualsLiteral("something") >+ // If possible, determine the MIME type from the request and use it as a failsafe. >+ nsCOMPtr<nsIChannel> channel(do_QueryInterface(aRequest)); Places usually uses the "=" code style nsCOMPtr<nsIChannel> channel = do_Query... >diff --git a/toolkit/components/places/tests/unit/test_download_favicons.js b/toolkit/components/places/tests/unit/test_download_favicons.js >+var icons = PlacesUtils.favicons; >+var dh = Cc["@mozilla.org/browser/download-history;1"]. >+ getService(Ci.nsIDownloadHistory); As i said, please add a PlacesUtils.downloadhistory lazy getter. + icons.expireAllFavicons(); + do_test_finished(); and PlacesUtils.bhistory.removeAllPages(); >+ QueryInterface: XPCOMUtils.generateQI([Ci.nsINavHistoryObserver]), >+}; >+var currentIndex = 0; newline between the observer and this var >+var tests = [ >+ { url: "file:///test.png" }, >+ { url: "file:///test.dmg" }, >+ { url: "file:///test.gif" }]; "];" on a new line and you can add a trailing comma on the last element if you wish >+// main kill this comment >+function run_test() { >+ do_test_pending(); >+ PlacesUtils.history.addObserver(observer, false); Where do you remove this observer? It should be removed once you don't need it anymore. Ext devs use tests as examples and we don't want to give bad examples >+} >+function startNextTest() { newline above method r=me with the above, try server is your friend.

This would also help responsiveness when opening dlmgr for the first time after a restart, which currently does a synchronous :( read of each file icon for downloaded files.

(In reply to comment #12) > This would also help responsiveness when opening dlmgr for the first time after > a restart, which currently does a synchronous :( read of each file icon for > downloaded files. Oh man, this would be sweet to land then. My original plan was to land this along with bug 564900 after mozilla 2.0, as bug 564900 has an API change. Would it make sense to try and land this sooner?

(In reply to comment #13) > Would it make sense to try and land this sooner? Probably not, but it's certainly something we can take after we branch. Having all this ready to go (I think this patch is bitrotted at this point) would be useful.

I have un-bitrotted the previous patch and the tests run fine on my machine. Here is a link to the tryserver build: http://tbpl.mozilla.org/?tree=Try&rev=aff965cb9f59

The previous try server build failed and I noticed two lines that were commented out but shouldn't have been. I have fixed the code and re-run the tests locally with positive results. Here is a link to the try server build for this patch: http://tbpl.mozilla.org/?tree=Try&rev=ac2ec4aa0535

mak: The try server builds still complete with multiple test failures. I am unable to reproduce these test failures locally. Do you have any idea why they may be failing on try?

Comment on attachment 546841 [details] [diff] [review] Patch for bug 564916 Review of attachment 546841 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/AsyncFaviconHelpers.cpp @@ +286,5 @@ > + * content types which can be used as an icon and which are not provided by > + * GetMIMETypeFromContent. > + */ > +static PRBool > +isContentTypeInWhitelist(const nsCString& aContentType) let's return a simple bool here @@ +292,5 @@ > + if (aContentType.EqualsLiteral("image/icon") || > + aContentType.EqualsLiteral("image/x-icon")) > + return PR_TRUE; > + > + return PR_FALSE; and at that point just return aContentType.EqualsLiteral("image/icon") || aContentType.EqualsLiteral("image/x-icon"); @@ +349,5 @@ > (void)sniffer->GetMIMETypeFromContent(aRequest, TO_INTBUFFER(aData), > aData.Length(), _mimeType); > + > + if (_mimeType.IsEmpty() && !contentType.IsEmpty()) { > + _mimeType.Assign(contentType.get()); _mimeType = contentType should work and be cheaper by just addrefing the buffer ::: toolkit/components/places/nsNavHistory.cpp @@ +5260,5 @@ > return NS_OK; > > PRInt64 visitID; > + nsresult rv = AddVisit(aSource, aStartTime, aReferrer, TRANSITION_DOWNLOAD, > + PR_FALSE, 0, &visitID); double whitespace after rv = Could you please file a Places bug to make addDownload async? using AddVisit here sucks, we should fix it asap.

(In reply to comment #18) > Could you please file a Places bug to make addDownload async? using AddVisit > here sucks, we should fix it asap. to clarify, we don't need to fix it now, I'm just willing to track it for future.

Fixed the nits. mak: Can you please double-check the tests on your machine and land it if they succeed? I'm not sure why try server is failing the tests.

So, this fails only on Linux, I have some suspect regarding moz-icon, we already noticed in the past there is a problem with the icon uris in xpcshell tests since they don't have a valid display, see the discussion in bug 660206. Fixing bug 660206 comment 20 may solve this issue as well. Jared are you willing to take a look at that?

Unassigning since I haven't been working on this.

Don't think it's still needed with the new download view.

Indeed. Current AddDownload API already takes the target file uri, so the new downloads view uses it for "generating" the moz-icon:// uri. Apart that, the approach used in the patch provided (storing the mime-type-icon as the favicon for the source uri) is very wrong. In particular, it would result in somewhat confusing results when pages are downloaded via Save Page As.