Closed Bug 521452 Opened 13 years ago Closed 13 years ago

nsUpdateService and nsBlocklistService both have getFile(key, pathArray)

Categories

(Toolkit :: Application Update, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta2-fixed

People

(Reporter: dietrich, Assigned: robert.strong.bugs)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [code cleanup])

Attachments

(1 file, 3 obsolete files)

should share the code somehow, via include or module.
Whiteboard: [code cleanup]
OS: Windows NT → All
Hardware: x86 → All
You mindreader... I was talking about how we should do this with Mossop yesterday!
hehe, cosmic ;)

nsExtensionManager has it as well.
No longer blocks: 521956
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.
Attached patch patch in progress (obsolete) — Splinter Review
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
Attached patch patch in progress rev2 (obsolete) — Splinter Review
Attachment #406190 - Attachment is obsolete: true
Attachment #406195 - Flags: review?(dtownsend)
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.
Attached patch patch rev2 (obsolete) — Splinter Review
Attachment #406195 - Attachment is obsolete: true
Attachment #407839 - Flags: review?(dtownsend)
Attachment #406195 - Flags: review?(dtownsend)
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.
Attached patch patch rev3Splinter Review
(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
Attachment #407839 - Attachment is obsolete: true
Attachment #407884 - Flags: review?(dtownsend)
Attachment #407839 - Flags: review?(dtownsend)
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.
Attachment #407884 - Flags: review?(dtownsend) → review+
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?
Attachment #407884 - Flags: superreview?(benjamin)
Attachment #407884 - Flags: superreview?(benjamin) → superreview+
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.
Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/daf4894395c9
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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.
Attachment #407884 - Flags: approval1.9.2?
Comment on attachment 407884 [details] [diff] [review]
patch rev3

Blocks refactor bug - should have been a+'d with the others
Attachment #407884 - Flags: approval1.9.2? → approval1.9.2+
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
Flags: in-testsuite+
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.
Adding dev-doc-needed to get FileUtils.jsm documented on MDC.
Keywords: dev-doc-needed
(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
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.