Last Comment Bug 707094 - DOMApplicationRegistry (Webapps.jsm): Use FileUtils.jsm for simpler and more robust nsIFile handling
: DOMApplicationRegistry (Webapps.jsm): Use FileUtils.jsm for simpler and more ...
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla11
Assigned To: [:fabrice] Fabrice Desré
: Andrew Overholt [:overholt]
Depends on:
Blocks: 697383
  Show dependency treegraph
Reported: 2011-12-02 00:39 PST by Philipp von Weitershausen [:philikon]
Modified: 2012-02-01 13:59 PST (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (3.79 KB, patch)
2011-12-05 19:59 PST, [:fabrice] Fabrice Desré
philipp: review+
Details | Diff | Splinter Review

Description User image Philipp von Weitershausen [:philikon] 2011-12-02 00:39:35 PST
In DOMApplicationRegistry, which resides in Webapps.jsm:

>    let file =  Cc[";1"].getService(Ci.nsIProperties).get("ProfD", Ci.nsIFile);
>    file.append("webapps");
>    if (!file.exists() || !file.isDirectory()) {
>      file.create(Ci.nsIFile.DIRECTORY_TYPE, 0700);
>    }
>    this.appsDir = file;
>    this.appsFile = file.clone();
>    this.appsFile.append("webapps.json");

First off, tsk tsk for not using Services.dirSvc. But, this whole thing can be written in just one line:

  let appsFile = FileUtils.getFile("ProfD", ["webapps", "webapps.json"], true);

The only difference is that the "webapps" directory may not be created with the desired 0700 permissions. Is this *that* important? The code gives no clue as to why you would want to protect that directory specially. (Also, magic constants should be const'ed!)

>  _writeFile: function ss_writeFile(aFile, aData, aCallbak) {
>    // Initialize the file output stream.
>    let ostream = Cc[";1"].createInstance(Ci.nsIFileOutputStream);
>    ostream.init(aFile, 0x02 | 0x08 | 0x20, 0600, ostream.DEFER_OPEN);

Again, tsk tsk for not const'ing the magic constants here. And, as usual, FileUtils will reduce this to one line:

  let ostream = FileUtils.openSafeFileOutputStream(aFile);

Once again, it gives no clue as to why the 0600 permission is requested here. FileUtils.openSafeFileOutputStream() will happily take different mode flags, but not different permissions yet. Though that could easily be added, and I would definitely welcome ways that would make FileUtils more useful.
Comment 1 User image [:fabrice] Fabrice Desré 2011-12-05 19:59:25 PST
Created attachment 579204 [details] [diff] [review]

Cleaned up with FileUtils. Yay for less code!
Comment 2 User image Philipp von Weitershausen [:philikon] 2011-12-05 20:02:20 PST
Comment on attachment 579204 [details] [diff] [review]

Comment 3 User image [:fabrice] Fabrice Desré 2011-12-05 20:25:00 PST
Comment 4 User image Ed Morley [:emorley] 2011-12-06 10:05:49 PST

Note You need to log in before you can comment on or make changes to this bug.