Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add FileSystemHandle::remove() method #283

Closed
wants to merge 4 commits into from
Closed

Add FileSystemHandle::remove() method #283

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
88638fd
Add remove() to spec
a-sully Mar 10, 2021
8bf2b58
Fix bs error
a-sully Mar 12, 2021
9858a15
update removeEntry error descriptions
a-sully Jun 8, 2021
9429f48
use DOMExceptions + add recursive option
a-sully Jun 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions index.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,8 @@ interface FileSystemHandle {
readonly attribute FileSystemHandleKind kind;
readonly attribute USVString name;

Promise<void> remove();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should have a "recurse: true/false" option analogous to the one removeEntry has. For files it wouldn't matter if you set it or not, but for directories it would make the difference between deleting the directory even when not empty, or only allowing deletion of empty directories.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, it would be odd to only allow recursive removals via removeEntry. I'll prototype this.


Promise<boolean> isSameEntry(FileSystemHandle other);

Promise<PermissionState> queryPermission(optional FileSystemHandlePermissionDescriptor descriptor = {});
Expand Down Expand Up @@ -315,6 +317,50 @@ and return {{FileSystemHandleKind/"directory"}} otherwise.
The <dfn attribute for=FileSystemHandle>name</dfn> attribute must return the [=entry/name=] of the
associated [=FileSystemHandle/entry=].

### The {{FileSystemHandle/remove()}} method ### {#api-filesystemhandle-remove}

<div class="note domintro">
: await |handle| . {{FileSystemHandle/remove()|remove}}()
:: Attempts to remove the entry represented by |handle| from the underlying file system.

For files or directories with multiple hardlinks or symlinks, the entry which is deleted
is the entry corresponding to the path which was used to obtain the handle.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pwnall This text was snagged from your comment here. Let me know if this needs more elaboration #214 (comment)

</div>

<div algorithm>
The <dfn method for=FileSystemHandle>remove()</dfn> method, when invoked, must run
these steps:

1. Let |result| be [=a new promise=].
1. Run the following steps [=in parallel=]:
1. Let |entry| be <b>[=this=]</b>'s [=FileSystemHandle/entry=].
1. Let |permissionStatus| be the result of [=requesting file system permission=]
given <b>[=this=]</b> and {{"readwrite"}}.
If that throws an exception, [=reject=] |result| with that exception and abort.
1. If |permissionStatus| is not {{PermissionState/"granted"}},
[=/reject=] |result| with a {{NotAllowedError}} and abort.

1. Attempt to remove |entry| from the underlying file system.
1. If |entry| is a [=directory entry=]:
1. If |entry|'s path does not exists, [=/reject=] |result| with a
{{NotFoundError}} and abort
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These errors aren't linked properly. Not sure the best way to describe these

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://heycam.github.io/webidl/#idl-DOMException-error-names lists the error names that exist on the platform (and technically everywhere in this spec I wrote things like reject with a {{NotAllowedError}}, that should really say reject with a {{"NotAllowedError"}} {{DOMException}} or something like that).

So the other error types below are not error types that actually exist, so presumably isn't what our draft implementation does either?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I would probably just use similar language as the current language for removeEntry (or ideally make both of them more precise about the errors that can be thrown under which circumstances, I'm not terribly happy with how things currently are defined there either but being consistent is probably better than having completely different language in both places. Ideally for handles in the origin private file system we should be able to exactly define the behavior in terms of concepts defined in this spec).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I should have been more clear here. {{NotFoundError}} is not an issue, but {{NotADirectoryError}}, {{NotAFileError}}, and {{NotEmptyError}} are. These are file errors [1], not DOM exceptions. These are converted to DOMExceptions [2], but some information is lost in translation. The first one here is particularly bad:

FILE_ERROR_NOT_EMPTY_ERROR => DOMExceptionCode::kInvalidModificationError
FILE_ERROR_NOT_A_FILE      => DOMExceptionCode::kTypeMismatchError
FILE_ERROR_NOT_A_DIRECTORY => DOMExceptionCode::kTypeMismatchError

The DOMException translations listed here are what should tentatively go in the spec, but there's a TODO [3] for supporting custom messages which are more applicable. While that would be useful it doesn't seem like it should be a blocker here?

[1] https://source.chromium.org/chromium/chromium/src/+/main:base/files/file.h;l=88
[2] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/fileapi/file_error.cc;l=145
[3] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/file_system_access/file_system_access_error.cc;l=62

1. If |entry|'s path is not a directory, [=/reject=] |result| with a
{{NotADirectoryError}} and abort
1. If |entry| is not an empty directory, [=/reject=] |result| with a
{{NotEmptyError}} and abort

1. If |entry| is a [=file entry=]:
1. If |entry|'s path does not exists, [=/reject=] |result| with a
{{NotFoundError}} and abort
1. If |entry|'s path is not a file, [=/reject=] |result| with a
{{NotAFileError}} and abort

1. [=/Resolve=] |result| with `undefined`.

1. Return |result|.

</div>

### The {{FileSystemHandle/isSameEntry()}} method ### {#api-filesystemhandle-issameentry}

<div class="note domintro">
Expand Down