961282 - In Webapps.jsm, create directories only when really needed

Status: RESOLVED WONTFIX

(Core Graveyard :: DOM: Apps, defect)

Description

[Marco Castelluccio [:marco]]

Attachment: webapps_create_dirs_only_when_needed

There are a lot of places where we create (or try to create) directories even if it is unneeded (for example, right before removing the directory).

This patch makes it clearer when we create directories (by not using FileUtils.getFile or FileUtils.getDir to create them [in most cases]) and removes all the useless creations.

The "webapps" directory is only created at startup.
The apps directories are only created in confirmInstall.
The tmp directories are only created when we download a package.

There are still a couple of usages of FileUtils.getFile, if you want I can get rid of them by using FileUtils.getDir and then nsIFile::create.

Comment 1

[[:fabrice] Fabrice Desré]

Comment on attachment 8361979 [details] [diff] [review]
webapps_create_dirs_only_when_needed

Review of attachment 8361979 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/apps/src/Webapps.jsm
@@ +402,5 @@
> +    try {
> +      destDir.create(Ci.nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
> +    } catch (ex if ex.result == Cr.NS_ERROR_FILE_ALREADY_EXISTS) {
> +      // Ignore the exception if the directory already exists.
> +    }

I'm not convinced we gain anything with this change.

@@ +2206,2 @@
>      }
>      aData.mm.sendAsyncMessage("Webapps:Install:Return:KO", aData);

could you send that once the removeDir promise is resolved in the |if (packageId)| branch?

@@ +2477,3 @@
>      let dir = this._getAppDir(aId);
>      zipFile.moveTo(dir, "application.zip");
> +    

nit: trailing whitespace

Comment 2

[Marco Castelluccio [:marco]]

Attachment: Patch

(In reply to Fabrice Desré [:fabrice] from comment #1)
> Comment on attachment 8361979 [details] [diff] [review]
> webapps_create_dirs_only_when_needed
> 
> Review of attachment 8361979 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/src/Webapps.jsm
> @@ +402,5 @@
> > +    try {
> > +      destDir.create(Ci.nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
> > +    } catch (ex if ex.result == Cr.NS_ERROR_FILE_ALREADY_EXISTS) {
> > +      // Ignore the exception if the directory already exists.
> > +    }
> 
> I'm not convinced we gain anything with this change.

I've used this pattern to avoid a call to |exists| (it's equivalent to OS.File.makeDir with the ignoreExisting option).

Comment 3

[[:fabrice] Fabrice Desré]

Comment on attachment 8377469 [details] [diff] [review]
Patch

Review of attachment 8377469 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Please run a try build before landing.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/96eaa0f376ab

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/96eaa0f376ab

Comment 6

[[:fabrice] Fabrice Desré]

backed out in https://hg.mozilla.org/integration/b2g-inbound/rev/a5758448882a for causing bug 975451

Comment 7

[Marco Castelluccio [:marco]]

The Apps API code has been removed in bug 1261019.