In the C++ implementation of bookmarks HTML 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. We should also consider whether it makes sense to change the favicon call to asynchronous when the bookmarks export is reimplemented in JavaScript, like it was done for the import.

Note how there's also a similar synchronous call in "test_bookmarks_html.js" to get the favicon data, nested in a series of for loops and function calls.

This is implemented on top of bug 763295, and makes the deeply nested favicon call asynchronous using a module called "Task.jsm", that currently exists on my local machine as a collage of code from various sources, based for the most part on "Task.js" <http://taskjs.org/>. This is real code that passes all tests.

Comment on attachment 631750 [details] [diff] [review] Make favicon calls asynchronous using a "Task.js"-style approach Marco's probably a better person to review this in general. I'll have to familiarize myself with Task.jsm a little more...

This has some minor updates compared to the previous version.

Comment on attachment 643097 [details] [diff] [review] Make favicon calls asynchronous using Task.jsm Review of attachment 643097 [details] [diff] [review]: ----------------------------------------------------------------- this clearly breaks autoExportHTML in browserGlue and any synchronous tests. see my comments in the cpp->js conversion bug first. That said, I like the task approach. ::: toolkit/components/places/BookmarkHTMLUtils.jsm @@ +21,5 @@ > + .createInstance(Ci.nsIStringInputStream); > + stream.setData(aString, aString.length); > + var encoder = Cc["@mozilla.org/scriptablebase64encoder;1"] > + .createInstance(Ci.nsIScriptableBase64Encoder); > + return encoder.encodeToString(stream, aString.length); nit: let! @@ +858,5 @@ > > exportToFile: function exportToFile(aLocalFile, aCallback) { > // Just execute the synchronous export function. > + let task = Task.spawn(this._doExportToFile(aLocalFile)); > + task.then( why do you need the task temp var, those methods are chainable iirc @@ +861,5 @@ > + let task = Task.spawn(this._doExportToFile(aLocalFile)); > + task.then( > + function (result) aCallback(true), > + function (ex) aCallback(false, ex) > + ); this should be moved to the original bug doing the cpp to js conversion for clarity I'd appreciate onSuccess and onError callbacks naming @@ +1126,5 @@ > } > }, > > + _promiseFaviconData: function(aPageURI) { > + var deferred = Promise.defer(); nit: let

(In reply to Marco Bonardo [:mak] from comment #5) > this should be moved to the original bug doing the cpp to js conversion > for clarity I'd appreciate onSuccess and onError callbacks naming Oh well, I lied here, there we can just simulate async with a simple enqueue, so nevermind.

This works neatly and passes all tests on top of bug 763295. I forgot to remove an unused aCallback argument there, I'm doing it here, since the patches will probably land at the same time.

Comment on attachment 683482 [details] [diff] [review] The patch Review of attachment 683482 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/BookmarkHTMLUtils.jsm @@ +96,5 @@ > + let stream = Cc["@mozilla.org/io/string-input-stream;1"] > + .createInstance(Ci.nsIStringInputStream); > + stream.setData(aString, aString.length); > + let encoder = Cc["@mozilla.org/scriptablebase64encoder;1"] > + .createInstance(Ci.nsIScriptableBase64Encoder); nit: align dots with [ @@ +1148,5 @@ > } > > this._write(" ICON_URI=\"" + this.escapeUrl(faviconURI.spec) + "\""); > > + if (faviconURI.scheme != "chrome" && dataLen > 0) { should probably use !faviconURI.schemeIs("chrome") @@ +1151,5 @@ > > + if (faviconURI.scheme != "chrome" && dataLen > 0) { > + let faviconContents = "data:image/png;base64," + > + base64EncodeString( > + String.fromCharCode.apply(String, data)); nit: just indent by 2 spaces and oneline

Is there a way I can test this?

export bookmarks to html and check the resulting file contains icons, I think.

I exported some bookmarks to .html file then imported them and the icons apear in Bookmark menu/Library. Tested this on latest Nightly, Aurora, FF19 beta 4.

Please reopen this if necessary, thx.