[OS.File] Create / remove empty directories

RESOLVED FIXED in mozilla17

Status

()

enhancement
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

({dev-doc-needed})

unspecified
mozilla17
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 18 obsolete attachments)

3.89 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
2.72 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
802 bytes, patch
Yoric
: review+
Details | Diff | Splinter Review
6.01 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
No description provided.
Summary: Implement directory removal in OS.File → [OS.File] Implement directory removal
After some tweaking around, I have come to the conclusion that we do not want to build recursive directory removal into OS.File yet. The main reason for this is that, depending on some information that OS.File does not have, we can have several very distinct implementations:
- move the directory to the OS-specific trash;
- move the directory to the OS-specific temporary directory;
- walk through the directory recursively, removing files as they come;
- walk through the directory recursively but asynchronously, removing a maximal number of files at a time, with a delay before two chunks;
- one of the two above combinations, but first move the directory to a kill-directory, that also needs to be emptied on startup;
- ...

For the moment, I therefore intend to only provide the features of moving directories/removing empty directories/walking through hierarchies. We will see for more advanced features once I have a clearer idea of the situation.
No longer depends on: 772538, 764436
Posted patch 1. The test suite (obsolete) — Splinter Review
Assignee: nobody → dteller
Posted patch 7. Constants (obsolete) — Splinter Review
Attachment #641881 - Attachment is patch: true
Attachment #641879 - Attachment description: Constants → 7. Constants
Summary: [OS.File] Implement directory removal → [OS.File] Create / remove empty directories
Posted patch mkdir/rmdir for Windows (obsolete) — Splinter Review
Attachment #641881 - Attachment is obsolete: true
Attachment #641883 - Attachment is obsolete: true
Attachment #641884 - Attachment is obsolete: true
Posted patch mkdir/rmdir for Unix (obsolete) — Splinter Review
Attachment #641880 - Attachment is obsolete: true
Attachment #641882 - Attachment is obsolete: true
Attachment #644317 - Flags: review?(taras.mozilla)

Updated

7 years ago
Attachment #644317 - Flags: review?(taras.mozilla) → review+
I have slightly changed my roadmap. Unless someone explicitly needs this, I will land this after bug 729057.
Assignee: dteller → nobody
Component: Networking: File → OS.File
Product: Core → Toolkit
Assignee: nobody → dteller
Posted patch mkdir/rmdir for Windows (obsolete) — Splinter Review
Attachment #644317 - Attachment is obsolete: true
Attachment #652432 - Flags: review?(nfroyd)
Posted patch mkdir/rmdir for Unix (obsolete) — Splinter Review
Attachment #644318 - Attachment is obsolete: true
Attachment #652434 - Flags: review?(nfroyd)
Posted patch The constants (obsolete) — Splinter Review
Attachment #641879 - Attachment is obsolete: true
Attachment #652436 - Flags: review?(nfroyd)
Posted patch Companion testsuite (obsolete) — Splinter Review
Attachment #641877 - Attachment is obsolete: true
Attachment #652437 - Flags: review?(nfroyd)
Comment on attachment 652432 [details] [diff] [review]
mkdir/rmdir for Windows

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

::: toolkit/components/osfile/osfile_win_front.jsm
@@ +328,5 @@
> +      *     directory does not exist yet.
> +      */
> +     File.removeEmptyDir = function removeEmptyDir(path, options) {
> +       options = options || noOptions;
> +       let result = UnixFile.rmdir(path);

Copy-paste-o?

@@ +346,5 @@
> +      * @param {*=} options Additional options. This
> +      * implementation interprets the following fields:
> +      *
> +      * - {number} winSecurity If specified, security attributes
> +      * as per winapi function |CreateDirectory|. If unspecified,

Wait, what?  The function takes a void* for security parameters and we merrily document that it takes numbers?  That seems mistaken.  The documentation should also be referring to |CreateDirectoryW|, no?
Attachment #652432 - Flags: review?(nfroyd) → feedback+
Attachment #652436 - Flags: review?(nfroyd) → review+
Attachment #652434 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) from comment #16)
> Comment on attachment 652432 [details] [diff] [review]
> mkdir/rmdir for Windows
> 
> Review of attachment 652432 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/osfile/osfile_win_front.jsm
> @@ +328,5 @@
> > +      *     directory does not exist yet.
> > +      */
> > +     File.removeEmptyDir = function removeEmptyDir(path, options) {
> > +       options = options || noOptions;
> > +       let result = UnixFile.rmdir(path);
> 
> Copy-paste-o?

Well-spotted, thanks.

> 
> @@ +346,5 @@
> > +      * @param {*=} options Additional options. This
> > +      * implementation interprets the following fields:
> > +      *
> > +      * - {number} winSecurity If specified, security attributes
> > +      * as per winapi function |CreateDirectory|. If unspecified,
> 
> Wait, what?  The function takes a void* for security parameters and we
> merrily document that it takes numbers?  That seems mistaken.

Documentation error, indeed.

> The
> documentation should also be referring to |CreateDirectoryW|, no?

Well, officially, according to Microsoft, the function is called |CreateDirectory|. The fact that it is #defined to |CreateDirectoryW| is an implementation trick that I do not want to document.
Posted patch mkdir/rmdir for Windows (obsolete) — Splinter Review
Attachment #652432 - Attachment is obsolete: true
Attachment #652463 - Flags: review?(nfroyd)
(In reply to David Rajchenbach Teller from comment #17)
> > @@ +346,5 @@
> > > +      * @param {*=} options Additional options. This
> > > +      * implementation interprets the following fields:
> > > +      *
> > > +      * - {number} winSecurity If specified, security attributes
> > > +      * as per winapi function |CreateDirectory|. If unspecified,
> > 
> > Wait, what?  The function takes a void* for security parameters and we
> > merrily document that it takes numbers?  That seems mistaken.
> 
> Documentation error, indeed.

Neither here nor there, but do we really have a standardized way of getting alternative security attributes to pass to the function?  If the answer is "no" or "do it via ctypes", that's ok.

> > The
> > documentation should also be referring to |CreateDirectoryW|, no?
> 
> Well, officially, according to Microsoft, the function is called
> |CreateDirectory|. The fact that it is #defined to |CreateDirectoryW| is an
> implementation trick that I do not want to document.

A pox on Microsoft's liberal use of #define'ing functions and whatnot in their headers.  Carry on.
Attachment #652463 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) from comment #19)
> (In reply to David Rajchenbach Teller from comment #17)
> > > @@ +346,5 @@
> > > > +      * @param {*=} options Additional options. This
> > > > +      * implementation interprets the following fields:
> > > > +      *
> > > > +      * - {number} winSecurity If specified, security attributes
> > > > +      * as per winapi function |CreateDirectory|. If unspecified,
> > > 
> > > Wait, what?  The function takes a void* for security parameters and we
> > > merrily document that it takes numbers?  That seems mistaken.
> > 
> > Documentation error, indeed.
> 
> Neither here nor there, but do we really have a standardized way of getting
> alternative security attributes to pass to the function?  If the answer is
> "no" or "do it via ctypes", that's ok.

For the moment, "do it via ctypes". Adding this at a later stage should be possible, if it becomes necessary.

> 
> > > The
> > > documentation should also be referring to |CreateDirectoryW|, no?
> > 
> > Well, officially, according to Microsoft, the function is called
> > |CreateDirectory|. The fact that it is #defined to |CreateDirectoryW| is an
> > implementation trick that I do not want to document.
> 
> A pox on Microsoft's liberal use of #define'ing functions and whatnot in
> their headers.  Carry on.

Yeah, multiple poxes on winAPI for just about everything. And don't get me started on paths :)
Comment on attachment 652437 [details] [diff] [review]
Companion testsuite

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

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
@@ +622,5 @@
> +{
> +  ok(true, "test_mkdir: Starting");
> +
> +  let dirName = "test_dir.tmp";
> +  OS.File.removeEmptyDir(dirName, {ignoreAbsent: true});

Checking that you throw for not-present ignoreAbsent in this case would be a good idea.

@@ +626,5 @@
> +  OS.File.removeEmptyDir(dirName, {ignoreAbsent: true});
> +
> +  ok(true, "test_mkdir: Creating directory");
> +  OS.File.makeDir(dirName);
> +  ok(true, OS.File.stat(dirName).isDir, "test_mkdir: Created directory is a directory");

What do you think about checking makeDir(dir) when dir already exists and/or is a file?

@@ +634,5 @@
> +  try {
> +    OS.File.stat(dirName);
> +    ok(false, "test_mkdir: Directory was not removed");
> +  } catch (x) {
> +    ok(x instanceof OS.File.Error && x.becauseNoSuchFile, "test_mkdir: Directory was correcly removed");

Just "test_mkdir: Directory was removed"; shorter and fixes the typo. :)

@@ +636,5 @@
> +    ok(false, "test_mkdir: Directory was not removed");
> +  } catch (x) {
> +    ok(x instanceof OS.File.Error && x.becauseNoSuchFile, "test_mkdir: Directory was correcly removed");
> +  }
> +

Checking that you throw for removing a non-empty directory would be good.  Checking that you throw for rmdir'ing a file would be good as well.
Attachment #652437 - Flags: review?(nfroyd) → feedback+
Posted patch Companion testsuite, v2 (obsolete) — Splinter Review
Here we go
Attachment #652437 - Attachment is obsolete: true
Attachment #652732 - Flags: review?(nfroyd)
Comment on attachment 652732 [details] [diff] [review]
Companion testsuite, v2

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

r=me with the changes below.

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
@@ +627,5 @@
> +
> +  // Check that removing absent directories is handled correctly
> +  let exn;
> +  try {
> +    OS.File.removeEmptyDir(dirName, {ignoreAbsent: true});

This looks like it's duplicating the test 5 lines earlier.  Why not just use the earlier one for the test or delete the earlier one?
Attachment #652732 - Flags: review?(nfroyd) → review+
Attachment #652434 - Attachment is obsolete: true
Attachment #653770 - Flags: review+
Posted patch mkdir/rmdir for Windows (obsolete) — Splinter Review
Attachment #652463 - Attachment is obsolete: true
Attachment #653771 - Flags: review+
Posted patch mkdir/rmdir for Windows (obsolete) — Splinter Review
Attachment #653771 - Attachment is obsolete: true
Attachment #653777 - Flags: review+
Posted patch The constants (obsolete) — Splinter Review
Attachment #652436 - Attachment is obsolete: true
Attachment #653779 - Flags: review+
Attachment #652732 - Attachment is obsolete: true
Attachment #653780 - Flags: review+
Looks like this is failing tests on Windows.
https://tbpl.mozilla.org/?tree=Try&rev=e0fee2e553ca

https://tbpl.mozilla.org/php/getParsedLog.php?id=14590527&tree=Try

13620 INFO TEST-PASS | chrome://mochitests/content/chrome/toolkit/components/osfile/tests/mochi/test_osfile_front.xul | test_mkdir: Creating directory that already exists
13621 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/components/osfile/tests/mochi/test_osfile_front.xul | test_mkdir: makeDir over an existing directory failed for all the right reasons
13622 INFO TEST-PASS | chrome://mochitests/content/chrome/toolkit/components/osfile/tests/mochi/test_osfile_front.xul | test_mkdir: Directory was removed
Keywords: checkin-needed
Investigating
Attachment #653779 - Attachment is obsolete: true
Attachment #654539 - Flags: review+
Attachment #653777 - Attachment is obsolete: true
Attachment #654540 - Flags: review+
You need to log in before you can comment on or make changes to this bug.