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

RESOLVED FIXED in mozilla9

Status

()

Toolkit
General
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

({dev-doc-complete})

Trunk
mozilla9
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

7 years ago
Created attachment 538231 [details] [diff] [review]
patch
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?
(Assignee)

Comment 4

7 years ago
Suits me, I'll do that.
(Assignee)

Comment 5

7 years ago
Created attachment 544944 [details] [diff] [review]
patch

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-
(Assignee)

Comment 7

7 years ago
Created attachment 558767 [details] [diff] [review]
patch v3

(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)
(Assignee)

Comment 8

7 years ago
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)
(Assignee)

Comment 9

7 years ago
Created attachment 559914 [details] [diff] [review]
patch v4

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+
(Assignee)

Updated

7 years ago
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
Last Resolved: 7 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
Documented:

https://developer.mozilla.org/en/JavaScript_code_modules/FileUtils.jsm#The_File_constructor

And mentioned on Firefox 9 for developers
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.