Closed
Bug 521452
Opened 16 years ago
Closed 16 years ago
nsUpdateService and nsBlocklistService both have getFile(key, pathArray)
Categories
(Toolkit :: Application Update, defect)
Toolkit
Application Update
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)
37.40 KB,
patch
|
mossop
:
review+
benjamin
:
superreview+
johnath
:
approval1.9.2+
|
Details | Diff | Splinter Review |
should share the code somehow, via include or module.
Reporter | ||
Updated•16 years ago
|
Whiteboard: [code cleanup]
Reporter | ||
Updated•16 years ago
|
OS: Windows NT → All
Hardware: x86 → All
![]() |
Assignee | |
Comment 1•16 years ago
|
||
You mindreader... I was talking about how we should do this with Mossop yesterday!
Reporter | ||
Comment 2•16 years ago
|
||
hehe, cosmic ;)
nsExtensionManager has it as well.
![]() |
Assignee | |
Comment 3•16 years ago
|
||
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.
![]() |
Assignee | |
Comment 4•16 years ago
|
||
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
![]() |
Assignee | |
Comment 5•16 years ago
|
||
Attachment #406190 -
Attachment is obsolete: true
![]() |
Assignee | |
Updated•16 years ago
|
Attachment #406195 -
Flags: review?(dtownsend)
Comment 6•16 years ago
|
||
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.
![]() |
Assignee | |
Comment 7•16 years ago
|
||
Attachment #406195 -
Attachment is obsolete: true
Attachment #407839 -
Flags: review?(dtownsend)
Attachment #406195 -
Flags: review?(dtownsend)
Comment 8•16 years ago
|
||
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.
![]() |
Assignee | |
Comment 9•16 years ago
|
||
(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 10•16 years ago
|
||
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+
![]() |
Assignee | |
Comment 11•16 years ago
|
||
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)
Updated•16 years ago
|
Attachment #407884 -
Flags: superreview?(benjamin) → superreview+
Comment 12•16 years ago
|
||
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.
![]() |
Assignee | |
Comment 13•16 years ago
|
||
Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/daf4894395c9
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
![]() |
Assignee | |
Comment 14•16 years ago
|
||
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 15•16 years ago
|
||
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+
![]() |
Assignee | |
Comment 16•16 years ago
|
||
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
status1.9.2:
--- → final-fixed
Flags: in-testsuite+
![]() |
Assignee | |
Comment 17•16 years ago
|
||
Forgot to fix the tab-width from comment #12 in the initial push
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8e2f0d06b25b
![]() |
Assignee | |
Comment 18•16 years ago
|
||
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.
![]() |
Assignee | |
Comment 19•16 years ago
|
||
Backout of the nsExtensionManager.js.in portion
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0c33b8654be4
Comment 20•14 years ago
|
||
Adding dev-doc-needed to get FileUtils.jsm documented on MDC.
Keywords: dev-doc-needed
Comment 21•14 years ago
|
||
(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
Updated•14 years ago
|
Keywords: dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•