Closed Bug 961282 Opened 6 years ago Closed 3 years ago

In Webapps.jsm, create directories only when really needed

Categories

(Core Graveyard :: DOM: Apps, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: marco, Assigned: marco)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attachment #8361979 - Flags: feedback?(fabrice)
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
Attachment #8361979 - Flags: feedback?(fabrice) → feedback+
Attached patch PatchSplinter Review
(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).
Assignee: nobody → mar.castelluccio
Attachment #8361979 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8377469 - Flags: review?(fabrice)
Comment on attachment 8377469 [details] [diff] [review]
Patch

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

r=me
Please run a try build before landing.
Attachment #8377469 - Flags: review?(fabrice) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/96eaa0f376ab
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla30 → ---
Depends on: 975451
The Apps API code has been removed in bug 1261019.
Status: REOPENED → RESOLVED
Closed: 6 years ago3 years ago
Resolution: --- → WONTFIX
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.