801618 - WebApps installer does not need asyncCopy

Status: RESOLVED FIXED

(Firefox Graveyard :: Web Apps, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

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

Comment 1

[Jonathan Vieyra]

Can you elaborate on the task to be completed?

Comment 2

[Berker Peksag [:berkerpeksag]]

Attachment: Patch v1

Comment 3

[Marco Castelluccio [:marco]]

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" });

Comment 4

[Marco Castelluccio [:marco]]

(In reply to Marco Castelluccio [:marco] from comment #3)
> You should remove this trailing whitespace.

Oh, sorry, it was there and you removed it :)

Comment 5

[Berker Peksag [:berkerpeksag]]

Attachment: Patch v2

Comment 6

[Marco Castelluccio [:marco]]

(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 :)

Comment 7

[Berker Peksag [:berkerpeksag]]

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

[David Teller [:Yoric] - still alive but not very active]

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 9

[David Teller [:Yoric] - still alive but not very active]

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 10

[Berker Peksag [:berkerpeksag]]

Attachment: Patch v3

Comment 11

[David Teller [:Yoric] - still alive but not very active]

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

Comment 12

[Berker Peksag [:berkerpeksag]]

Attachment: Patch v3 (rebased)

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

Comment 13

[David Teller [:Yoric] - still alive but not very active]

I think that the correct tests are actually on webapprt/test.

Pushed to Try, though: https://tbpl.mozilla.org/?tree=Try&rev=288ffadc2d84

Comment 14

[Berker Peksag [:berkerpeksag]]

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

Comment 15

[David Teller [:Yoric] - still alive but not very active]

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

Comment 16

[Berker Peksag [:berkerpeksag]]

Attachment: Patch v3 (rebased and ready for checkin)

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf2e08f56575

Thanks for the patch!

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cf2e08f56575

Comment 19

[Marco Castelluccio [:marco]]

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

Comment 20

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 21

[Marco Castelluccio [:marco]]

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