521452 - nsUpdateService and nsBlocklistService both have getFile(key, pathArray)

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Dietrich Ayala (:dietrich)]

should share the code somehow, via include or module.

Comment 1

[Robert Strong (they/them - no direct email)]

You mindreader... I was talking about how we should do this with Mossop yesterday!

Comment 2

[Dietrich Ayala (:dietrich)]

hehe, cosmic ;)

nsExtensionManager has it as well.

Comment 3

[Robert Strong (they/them - no direct email)]

So, one of the things Taras mentioned in Bug 311965 was that js_Execute takes noticeably longer on Maemo for larger files so I think these should be split out into a module as is done to remove the include of badCertHandler.js in bug 521956.

Comment 4

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress

Comment 5

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress rev2

Comment 6

[Dave Townsend [:mossop]]

Feels a bit odd to be exporting something called getDirInternal. Is it really necessary or can we switch callers to getDir and getDirNoCreate? Also I think we should export the MODE and PERMS values rather than redefining those over and over again.

Comment 7

[Robert Strong (they/them - no direct email)]

Attachment: patch rev2

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 407839 [details] [diff] [review]
patch rev2

>diff --git a/toolkit/mozapps/shared/src/FileUtils.jsm b/toolkit/mozapps/shared/src/FileUtils.jsm

>+  /**
>+   * Gets the file at the specified hierarchy under a Directory Service key.
>+   * @param   key
>+   *          The Directory Service Key to start from
>+   * @param   pathArray
>+   *          An array of path components to locate beneath the directory
>+   *          specified by |key|. The last item in this array must be the
>+   *          leaf name of a file.
>+   * @return  nsIFile object for the file specified. The file is NOT created
>+   *          if it does not exist, however all required directories along
>+   *          the way are.
>+   */
>+  getFile: function FileUtils_getFile(key, pathArray, followLinks) {
>+    var file = this.getDir(key, pathArray.slice(0, -1), false, followLinks);
>+    file.append(pathArray[pathArray.length - 1]);
>+    return file;
>+  },

The comment says directories along the way are created yet you pass false for shouldCreate to getDir.

>+  /**
>+   * Opens a safe file output stream for writing.
>+   * @param   file
>+   *          The file to write to.
>+   * @param   modeFlags
>+   *          (optional) File open flags. Can be undefined.
>+   * @returns nsIFileOutputStream to write to.
>+   */
>+  openSafeFileOutputStream: function FileUtils_openSafeFileOutputStream(file, modeFlags) {
>+    var fos = Cc["@mozilla.org/network/safe-file-output-stream;1"].
>+              createInstance(Ci.nsIFileOutputStream);
>+    if (modeFlags === undefined)
>+      modeFlags = this.MODE_WRONLY | this.MODE_CREATE | this.MODE_TRUNCATE;
>+    if (!file.exists())
>+      file.create(Ci.nsILocalFile.NORMAL_FILE_TYPE, this.PERMS_FILE);

From the old code I know, but why is it we create the file manually first rather than relying on our mode flags to do that for us?

>+    fos.init(file, modeFlags, this.PERMS_FILE, 0);
>+    return fos;
>+  },

Wonder if it'd be useful to add a matching openFileOutputStream function too for the simpler case.

>diff --git a/toolkit/mozapps/shared/src/Makefile.in b/toolkit/mozapps/shared/src/Makefile.in
>--- a/toolkit/mozapps/shared/src/Makefile.in
>+++ b/toolkit/mozapps/shared/src/Makefile.in
>@@ -46,10 +46,12 @@ MODULE = toolkitShared
> 
> EXTRA_JS_MODULES = \
>   CertUtils.jsm \
>+  FileUtils.jsm \
>   $(NULL)
> 
> EXTRA_PP_JS_MODULES = \
>   CertUtils.jsm \
>+  FileUtils.jsm \
>   $(NULL)

Missed this before, but you shouldn't need either of these modules in EXTRA_JS_MODULES. Just EXTRA_PP_JS_MODULES on its own should work.

Comment 9

[Robert Strong (they/them - no direct email)]

Attachment: patch rev3

(In reply to comment #8)
> (From update of attachment 407839 [details] [diff] [review])
> >diff --git a/toolkit/mozapps/shared/src/FileUtils.jsm b/toolkit/mozapps/shared/src/FileUtils.jsm
> 
> The comment says directories along the way are created yet you pass false for
> shouldCreate to getDir.
Fixed... changed to true

> From the old code I know, but why is it we create the file manually first
> rather than relying on our mode flags to do that for us?
Not sure why but other consumers of safe-file-output-stream don't so I removed this.

> Wonder if it'd be useful to add a matching openFileOutputStream function too
> for the simpler case.
Perhaps a followup bug?

> Missed this before, but you shouldn't need either of these modules in
> EXTRA_JS_MODULES. Just EXTRA_PP_JS_MODULES on its own should work.
Fixed

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 407884 [details] [diff] [review]
patch rev3

Fine by me, should find out if bsmedberg needs to do an sr of this though.

Comment 11

[Robert Strong (they/them - no direct email)]

Comment on attachment 407884 [details] [diff] [review]
patch rev3

Benjamin, this adds a FileUtils.jsm that is shared by app update, extension manager, and blocklist. Can I get an sr for this from you?

Comment 12

[Benjamin Smedberg]

Comment on attachment 407884 [details] [diff] [review]
patch rev3

>diff --git a/toolkit/mozapps/shared/src/FileUtils.jsm b/toolkit/mozapps/shared/src/FileUtils.jsm


>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Per the style guide, this should be tab-width 8, makes it easier to see sneaky hard tabs that got added accidentally.

Comment 13

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/daf4894395c9

Comment 14

[Robert Strong (they/them - no direct email)]

Comment on attachment 407884 [details] [diff] [review]
patch rev3

Drivers, this is part of the work heading up to fixing bug 311965 which should improve TS on fennec and Firefox WinCE... possibly a few ms on desktop as well.

Comment 15

[Johnathan Nightingale [:johnath]]

Comment on attachment 407884 [details] [diff] [review]
patch rev3

Blocks refactor bug - should have been a+'d with the others

Comment 16

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ea45cbc526dc

Adding in-testsuite+ since this code is heavily exercised by both app update and extension manager xpcshell tests

Comment 17

[Robert Strong (they/them - no direct email)]

Forgot to fix the tab-width from comment #12 in the initial push
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8e2f0d06b25b

Comment 18

[Robert Strong (they/them - no direct email)]

I had to backout the extension manager portion of this patch on 1.9.2 (works on trunk) due to reftest failures. I filed bug 526441 to figure out what is going on and to fix it.

Comment 19

[Robert Strong (they/them - no direct email)]

Backout of the nsExtensionManager.js.in portion
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0c33b8654be4

Comment 20

[Shawn Wilsher :sdwilsh]

Adding dev-doc-needed to get FileUtils.jsm documented on MDC.

Comment 21

[Philipp von Weitershausen [:philikon]]

(In reply to comment #20)
> Adding dev-doc-needed to get FileUtils.jsm documented on MDC.

You can haz dox: https://developer.mozilla.org/en/JavaScript_code_modules/FileUtils.jsm