Closed Bug 801618 Opened 9 years ago Closed 9 years ago

WebApps installer does not need asyncCopy

Categories

(Firefox Graveyard :: Web Apps, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 22

People

(Reporter: Yoric, Assigned: berkerpeksag)

References

(Blocks 1 open bug)

Details

(Whiteboard: [mentor=Yoric][lang=js])

Attachments

(1 file, 4 obsolete files)

Whiteboard: [mentor=Yoric][lang=js]
Can you elaborate on the task to be completed?
Attached patch Patch v1 (obsolete) — Splinter Review
Assignee: nobody → berker.peksag
Status: NEW → ASSIGNED
Attachment #712084 - Flags: feedback?(dteller)
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 :)
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #712084 - Attachment is obsolete: true
Attachment #712084 - Flags: feedback?(dteller)
Attachment #712259 - Flags: review?(mar.castelluccio)
(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 :)
Attachment #712084 - Attachment is obsolete: false
(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 :)
Attachment #712084 - Flags: feedback?(dteller)
Attachment #712259 - Flags: review?(mar.castelluccio) → feedback?(dteller)
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.
Attachment #712084 - Flags: feedback?(dteller) → feedback+
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|.
Attachment #712259 - Flags: feedback?(dteller) → feedback+
Attached patch Patch v3 (obsolete) — Splinter Review
Attachment #712084 - Attachment is obsolete: true
Attachment #712259 - Attachment is obsolete: true
Attachment #712868 - Flags: review?(dteller)
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
Attachment #712868 - Flags: review?(dteller) → review+
Attached patch Patch v3 (rebased) (obsolete) — Splinter Review
(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?
Flags: needinfo?(dteller)
I think that the correct tests are actually on webapprt/test.

Pushed to Try, though: https://tbpl.mozilla.org/?tree=Try&rev=288ffadc2d84
Flags: needinfo?(dteller)
(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
Attachment #712868 - Attachment is obsolete: true
Attachment #715266 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cf2e08f56575
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Yoric, the other asyncCopy functions are being used to copy PNG images. Is it better to use asyncCopy in those cases?
Flags: needinfo?(dteller)
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.
Flags: needinfo?(dteller)
(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.
Depends on: 901757
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.