Closed Bug 663075 Opened 9 years ago Closed 9 years ago

FileUtils.jsm should have an easy way to create an nsILocalFile with a path

Categories

(Toolkit :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: darktrojan, Assigned: darktrojan)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 3 obsolete files)

No description provided.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Attachment #538231 - Flags: review?(sdwilsh)
Comment on attachment 538231 [details] [diff] [review]
patch

Drive-by review...

>+  getFileWithPath: function FileUtils_getFileWithPath(aPath) {
>+    let file = Cc ["@mozilla.org/file/local;1"].createInstance (Ci.nsILocalFile);

Nit: get rid of the spaces before [ and (

>+add_test(function test_getFileWithPath() {
>+  let testpath = FileUtils.getFile("ProfD", ["test"]).path;
>+  let file = FileUtils.getFileWithPath(testpath);
>+  do_check_true(file instanceof Components.interfaces.nsILocalFile);
>+  do_check_eq(file.path, testpath);

You could also do file.equals(otherfile) where otherfile is the one created via FileUtils.getFile().
Comment on attachment 538231 [details] [diff] [review]
patch

Review of attachment 538231 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/shared/FileUtils.jsm
@@ +138,5 @@
>      }
>      stream.close();
> +  },
> +
> +  getFileWithPath: function FileUtils_getFileWithPath(aPath) {

I think I'd rather expose an object that does the magic here:
let file = new FileUtils.File("/path/to/file");
instead of:
let file = FileUtils.getFileWithPath("/path/to/file");

Feels more JavaScripty and less XPCOMy to me at least.  Thoughts?
Suits me, I'll do that.
Attached patch patch (obsolete) — Splinter Review
Not sure why I didn't attach this earlier. Hmm.
Attachment #538231 - Attachment is obsolete: true
Attachment #538231 - Flags: review?(sdwilsh)
Attachment #544944 - Flags: review?(sdwilsh)
Comment on attachment 544944 [details] [diff] [review]
patch

Review of attachment 544944 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry this has taken so long!  Should be a snappy review with these comments addressed.  Then this will need sr from :rs

::: toolkit/mozapps/shared/FileUtils.jsm
@@ +159,5 @@
>      }
>      stream.close();
> +  },
> +
> +  File: function FileUtils_File(aPath) {

I think you actually just want to make `File` a `Components.Constructor` object: https://developer.mozilla.org/en/Components.constructor

::: toolkit/mozapps/shared/test/unit/test_FileUtils.js
@@ +189,5 @@
> +  let testpath = testfile.path;
> +  let file = new FileUtils.File(testpath);
> +  do_check_true(file instanceof Components.interfaces.nsILocalFile);
> +  do_check_true(file.equals(testfile));
> +  do_check_neq(file, testfile);

I don't think this check is useful.
Attachment #544944 - Flags: review?(sdwilsh) → review-
Attached patch patch v3 (obsolete) — Splinter Review
(In reply to Shawn Wilsher :sdwilsh from comment #6)
> I think you actually just want to make `File` a `Components.Constructor`
> object: https://developer.mozilla.org/en/Components.constructor

Ooh, I just learned a new trick!

> ::: toolkit/mozapps/shared/test/unit/test_FileUtils.js
> @@ +189,5 @@
> > +  let testpath = testfile.path;
> > +  let file = new FileUtils.File(testpath);
> > +  do_check_true(file instanceof Components.interfaces.nsILocalFile);
> > +  do_check_true(file.equals(testfile));
> > +  do_check_neq(file, testfile);
> 
> I don't think this check is useful.

I'm assuming you mean just the last line and splinter gave 4 lines of context just to be confusing, right?
Attachment #544944 - Attachment is obsolete: true
Attachment #558767 - Flags: review?(sdwilsh)
Comment on attachment 558767 [details] [diff] [review]
patch v3

There's a stray semicolon in my patch. I wonder why I never noticed it? :(
Attachment #558767 - Attachment is obsolete: true
Attachment #558767 - Flags: review?(sdwilsh)
Attached patch patch v4Splinter Review
Try again, shall we?
Attachment #559914 - Flags: review?(sdwilsh)
Comment on attachment 559914 [details] [diff] [review]
patch v4

r=sdwilsh
Attachment #559914 - Flags: superreview?(robert.bugzilla)
Attachment #559914 - Flags: review?(sdwilsh)
Attachment #559914 - Flags: review+
Attachment #559914 - Flags: superreview?(robert.bugzilla) → superreview+
Keywords: checkin-needed
This is now in my queue of things that are being sent to try then onto inbound :-)
Keywords: checkin-needed
Version: unspecified → Trunk
https://hg.mozilla.org/mozilla-central/rev/ebbc50bbebeb
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.