Closed
Bug 521452
Opened 14 years ago
Closed 14 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•14 years ago
|
Whiteboard: [code cleanup]
Reporter | ||
Updated•14 years ago
|
OS: Windows NT → All
Hardware: x86 → All
![]() |
Assignee | |
Comment 1•14 years ago
|
||
You mindreader... I was talking about how we should do this with Mossop yesterday!
Reporter | ||
Comment 2•14 years ago
|
||
hehe, cosmic ;) nsExtensionManager has it as well.
![]() |
Assignee | |
Comment 3•14 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•14 years ago
|
||
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
![]() |
Assignee | |
Comment 5•14 years ago
|
||
Attachment #406190 -
Attachment is obsolete: true
![]() |
Assignee | |
Updated•14 years ago
|
Attachment #406195 -
Flags: review?(dtownsend)
Comment 6•14 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•14 years ago
|
||
Attachment #406195 -
Attachment is obsolete: true
Attachment #407839 -
Flags: review?(dtownsend)
Attachment #406195 -
Flags: review?(dtownsend)
Comment 8•14 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•14 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•14 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•14 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•14 years ago
|
Attachment #407884 -
Flags: superreview?(benjamin) → superreview+
Comment 12•14 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•14 years ago
|
||
Pushed to mozilla-central http://hg.mozilla.org/mozilla-central/rev/daf4894395c9
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
![]() |
Assignee | |
Comment 14•14 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•14 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•14 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•14 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•14 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•14 years ago
|
||
Backout of the nsExtensionManager.js.in portion http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0c33b8654be4
Comment 20•13 years ago
|
||
Adding dev-doc-needed to get FileUtils.jsm documented on MDC.
Keywords: dev-doc-needed
Comment 21•13 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•13 years ago
|
Keywords: dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•