See http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/WebappsInstaller.jsm#885

Can you elaborate on the task to be completed?

Comment on attachment 712084 [details] [diff] [review] Patch v1 Review of attachment 712084 [details] [diff] [review]: ----------------------------------------------------------------- Yoric, the other asyncCopy functions are being used to copy PNG images. Is it better to use asyncCopy in those cases? ::: toolkit/webapps/WebappsInstaller.jsm @@ +342,5 @@ > writer.setString("TASKBAR", "Migrated", "true"); > writer.writeFile(null, Ci.nsIINIParserWriter.WRITE_UTF16); > > // ${UninstallDir}/uninstall.log > + let uninstallContent = You should remove this trailing whitespace. @@ +876,5 @@ > > /* Helper Functions */ > > /** > + * Atomic write a data string into a file I wouldn't change this comment. @@ +892,5 @@ > + promise.then( > + function onSuccess(x) { > + aCallback(x); > + } > + ); If you remove the aCallback argument (that isn't used by any caller), I think you can simply do something like this: > let data = new TextEncoder().encode(aData); > OS.File.writeAtomic(aFile.path, data, { tmpPath: aFile.path + ".tmp" });

(In reply to Marco Castelluccio [:marco] from comment #3) > You should remove this trailing whitespace. Oh, sorry, it was there and you removed it :)

(In reply to Berker Peksag [:berkerpeksag] from comment #5) > Created attachment 712259 [details] [diff] [review] > Patch v2 Hey Berker, my comments were only suggestions, you should ask Yoric to review your code :)

(In reply to Marco Castelluccio [:marco] from comment #6) > (In reply to Berker Peksag [:berkerpeksag] from comment #5) > > Created attachment 712259 [details] [diff] [review] > > Patch v2 > > Hey Berker, my comments were only suggestions, you should ask Yoric to > review your code :) Ah, okay. The whitespace change still out of scope for the bug, thanks for the noticing :)

Comment on attachment 712084 [details] [diff] [review] Patch v1 Review of attachment 712084 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: toolkit/webapps/WebappsInstaller.jsm @@ +876,5 @@ > > /* Helper Functions */ > > /** > + * Atomic write a data string into a file Agreed with marco. @@ +892,5 @@ > + promise.then( > + function onSuccess(x) { > + aCallback(x); > + } > + ); This is good, but I agree with marco's remark that we can take the opportunity to remove the callback from this function and its callers. In case we ever need the feature some day, just return the promise.

Comment on attachment 712259 [details] [diff] [review] Patch v2 Review of attachment 712259 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/webapps/WebappsInstaller.jsm @@ +884,5 @@ > * @param aCallback a callback to be called after the process is finished > */ > function writeToFile(aFile, aData, aCallback) { > + let data = new TextEncoder().encode(aData); > + OS.File.writeAtomic(aFile.path, data, { tmpPath: aFile.path + ".tmp" }); Please: - remove argument |aCallback|; - remove the third argument from all the call sites of |writeToFile|; - return the result of |writeAtomic|.

Comment on attachment 712868 [details] [diff] [review] Patch v3 Review of attachment 712868 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. r=me if it passes Try

(In reply to David Rajchenbach Teller [:Yoric] from comment #11) > Looks good to me. > r=me if it passes Try Thanks for the review, Yoric. I've rebased the patch and ran the tests in dom/tests/mochitest/webapps/. Is the location correct? I don't have a level 1 hg account to push the try server. Can you help me with this?

I think that the correct tests are actually on webapprt/test. Pushed to Try, though: https://tbpl.mozilla.org/?tree=Try&rev=288ffadc2d84

(In reply to David Rajchenbach Teller [:Yoric] from comment #13) > I think that the correct tests are actually on webapprt/test. > > Pushed to Try, though: https://tbpl.mozilla.org/?tree=Try&rev=288ffadc2d84 Thanks! Looks like these failures are not related with the patch. Should I add the "checkin-needed" keyword?

Looks good to me. You will need to add ";r=yoric" at the end of your commit message. See http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf2e08f56575 Thanks for the patch!

Yoric, the other asyncCopy functions are being used to copy PNG images. Is it better to use asyncCopy in those cases?

Taking a look at WebApps.jsm and WebappsIconHelpers.js, it seems that the remaining calls of asyncCopy are used to copy an icon that has already been downloaded to /tmp. In that case, just use OS.File.copy, which is much more efficient than asyncCopy.

(In reply to David Rajchenbach Teller [:Yoric] from comment #20) > Taking a look at WebApps.jsm and WebappsIconHelpers.js, it seems that the > remaining calls of asyncCopy are used to copy an icon that has already been > downloaded to /tmp. In that case, just use OS.File.copy, which is much more > efficient than asyncCopy. We're actually also converting the images, so I think asyncCopy should be necessary.