See bug 713642 for background.

this should stay on-hold till bug 482911 lands, to avoid code conflicts.

(In reply to Marco Bonardo [:mak] from comment #1) > this should stay on-hold till bug 482911 lands, to avoid code conflicts. Arrr... your guess that I was about to start working on this was right, but the message reached me too late! In any case I looked at the other patch, and the same changes can be applied to the JavaScript implementation as well as the tests there.

By the way, in the C++ implementation of bookmarks export, we make a synchronous call to get the favicon URI. That single call is nested in a series of "Write..." calls that are all cascaded synchronously. I don't see an easy way to change that. If we want to change that, we should probably do it in a separate bug, as it may even be easier to just reimplement the export part with asynchronous callbacks in JavaScript, like it was done for the import.

Also, have you considered changing "http://www.mozilla.org/2005/made-up-favicon/" to something like "urn:moz-made-up-favicon:"? If we use a fake URI, we might as well use one that doesn't hit the network if we fail to store the associated favicon data in the database.

if we use replaceFaviconData doesn't matter.

(In reply to Marco Bonardo [:mak] from comment #5) > if we use replaceFaviconData doesn't matter. What if replaceFaviconData fails?

we hopefully bail out earlier. Btw, I have no preferences in the fake icons url, we may do that, if we don't find points against it.

Comment on attachment 606232 [details] [diff] [review] Reference patch plase reflag me once bug 482911 and the patch is unbitrotted upon it

"once bug 482911 lands"

Comment on attachment 610584 [details] [diff] [review] The patch Review of attachment 610584 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/places/tests/unit/test_457441-import-export-corrupt-bookmarks-html.js @@ +84,5 @@ > > // Check that every bookmark is correct > // Corrupt bookmarks should not have been imported > + database_check(function () { > + waitForAsyncUpdates(function() { since you now do an async database check, you can avoid the wait. @@ +124,1 @@ > waitForAsyncUpdates(do_test_finished); as well as here, you can avoid the wait @@ +230,5 @@ > unfiledBookmarks.containerOpen = false; > > // favicons > + icos.getFaviconDataForPage(uri(TEST_FAVICON_PAGE_URL), > + function DC_onFaviconDataAvailable(aURI, aDataLen, aData, aMimeType) { sanity check aURI is not null ::: toolkit/components/places/BookmarkHTMLUtils.jsm @@ +621,5 @@ > // worry about data > if (aIconURI) { > if (aIconURI.scheme == "chrome") { > + PlacesUtils.favicons.setAndFetchFaviconForPage(aPageURI, aIconURI, > + false); may you please add a test for adding a chrome icon uri if we don't have one? @@ +657,5 @@ > + PlacesUtils.favicons.replaceFaviconDataFromDataURL(faviconURI, aData, 0); > + } catch (ex) { > + Cu.reportError(ex); > + } > + PlacesUtils.favicons.setAndFetchFaviconForPage(aPageURI, faviconURI, false); If we fail to store the data, there's no point in storing the icon, we'd just associate a new icon next time the page is visited, couldn't we just move this inside the try then, or early return? And then fix the above comment about hitting the network

Comment on attachment 611164 [details] [diff] [review] Updated patch >+ // aURI should never be null when aDataLen > 0. >+ do_check_true(aURI); Attached an old patch, this should read "do_check_neq(aURI, null)", "do_check_true(!!aURI)" or "do_check_true(aURI != null)". We use all these versions in the codebase. Preferences?

(In reply to Paolo Amadini [:paolo] from comment #13) > Attached an old patch, this should read "do_check_neq(aURI, null)", I like this one!

Comment on attachment 611164 [details] [diff] [review] Updated patch Review of attachment 611164 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/places/tests/unit/test_bookmarks_html.js @@ +106,5 @@ > > +/** > + * Returns the base64-encoded version of the given string. > + */ > +function xpcshell_btoa(aString) { hm, move this to the head? sounds like useful. I'd avoid the xpcshell prefix though, may confuse mxr searches. maybe just base64Encode. @@ +261,5 @@ > + // 5. import bookmarks.exported.html > + // 6. run the test-suite > + // 7. remove the bookmark pointing to a chrome favicon. > + // 8. export to bookmarks.exported.html > + // 9. empty bookmarks db and continue funny times eh. one day this should be splitted and cleaned up :( Maybe once we'll have a new export as well. ::: toolkit/components/places/BookmarkHTMLUtils.jsm @@ +651,4 @@ > // This could fail if the favicon is bigger than defined limit, in such a > + // case neither the favicon URI nor the favicon data will be saved. If the > + // bookmark is visited again later, the URI and data will be fetched. > + PlacesUtils.favicons.replaceFaviconDataFromDataURL(faviconURI, aData, 0); the last param is optional, thus you can avoid it.

(In reply to Marco Bonardo [:mak] from comment #15) > > +/** > > + * Returns the base64-encoded version of the given string. > > + */ > > +function xpcshell_btoa(aString) { > > hm, move this to the head? sounds like useful. I'd avoid the xpcshell prefix > though, may confuse mxr searches. maybe just base64Encode. I've called it base64EncodeString, for the poor guys who'll try to figure out where this magic function is located in our codebase :-)