Re: Polished FileSystem API proposal

On Mon, Jul 15, 2013 at 2:42 PM, Eric U <ericu@google.com> wrote:
> 75% bikeshedding, 25% lessons learned:
>
> It seems inconsistent to have the API for recursion done differently
> in two different parts of the API: you've got two functions for
> enumerate/enumerateDeep but remove takes a parameter to be recursive.
> I don't have a strong preference between them, but I think you should
> only use one style.

Good point. I'll change it to remove and removeDeep for now. That also
avoids the "what is default" question.

Out of curiosity, has it been a problem for you that the
DirectoryEntry API doesn't have a non-recursive remove option?
Granted, you do have the ability to grab a sub-Entry and call .remove
on it. We didn't have that option since we didn't want to create an
equivalent of FileEntry.

> What's the purpose of DestinationDict?  It seems redundant.

There's basically three potential ways to specify a copy/move destination:

Specify a new name within the current directory.
Specify a new directory but keep the current name.
Specify a new directory as well as a new name within that directory.

We obviously don't have to support them all, for example you could
imagine requiring a two-step process in some cases. But we decided to
try to support all three as a single operation for now.

Coming up with a syntax for this is somewhat tricky. In C++ you'd
simply have three overloads that take the different combinations, but
that doesn't feel very "javascripty".

Another is to use optional arguments like:

move(..., Directory? destDir, optional DOMString destName);

would work. That means that the three options would look like:

dir.move("srcfile", null, "newName");
dir.move("srcfile", destDir);
dir.move("srcfile", destDir, "newName");

Unfortunately the first one there looks pretty iffy. So instead we
went with a union which results in syntax like:

dir.move("srcfile", "newName");
dir.move("srcfile", destDir);
dir.move("srcfile", { dir: destDir. name: "newName" });

Another solution might be to only use dictionaries. Resulting in

dir.move("srcfile", { name: "newName" });
dir.move("srcfile", { dir: destDir });
dir.move("srcfile", { dir: destDir. name: "newName" });

I think I'd prefer to defer to TC39 about which option is best here.
I'll point this out explicitly when emailing public-script-coord.

> You have MakeFileOptions.overwriteIfExists for createFile, but
> CreateMode { createIfNeeded | dontcreate } for openWrite; it feels
> like those should share a set of flags.  The options should probably
> be {createIfNeeded, dontCreate, failIfExists}, where failIfExists
> implies createIfNeeded and is the standard POSIX paradigm
> [O_CREAT|O_EXCL].  It's useful for coordination between multiple pages
> in the same origin, or the same page loaded twice.

So you're basically proposing (modulo naming):

createFile("name", { mode: "createIfNeeded" });
Creates the file if it doesn't exist. Overwrites it if it does.

createFile("name", { mode: "dontCreate" });
Error if the file doesn't exist. Overwrites it if it does.

createFile("name", { mode: "failIfExists" });
Creates the file if it doesn't exist. Error it if it does.

openWrite("name", "createIfNeeded");
Creates and opens the file if it doesn't exist. Opens it if it does.

openWrite("name", "dontCreate");
Error if the file doesn't exist. Opens it if it does.

openWrite("name", "failIfExists");
Creates and opens the file if it doesn't exist. Error it if it does.

Is this correct? While I agree it's consistent, the number of
combinations here is a bit mind boggling. It took me quite a while to
write all of these out and make sure they are correct. The
createFile("dontCreate") combination is especially confusing.

> If you don't allow ".." to traverse directories, please define using
> it as a path segment as an error, to allow for future expansion and to
> catch bugs for folks who didn't read the spec.

Definitely.

> openForWrite or openForWriting, while longer, would be clearer than
> openWrite.  Heck, why not just "write"?

I tried out just "write" for a while, but it seemed strange that
myDir.write() actually started writing to a file.

> We've had folks ask for progress events on long move and copy calls.
> Think about network drive latencies, big SD cards being brought into
> the browser world, etc.  It's especially important if you think you'll
> ever use this API outside the sandbox, and cheap to add now even if
> you won't.  If you don't have it, people hack around it by
> reimplementing copy using write and createDirectory, and it's tedious
> and error-prone.  In your case, you'll probably want them for
> createFile as well, since you can pass in a gigantic Blob or
> ArrayBuffer there.

Hmm.. Good points. For directory copy/move operations we couldn't
provide a total size, but we could still provide progress in the form
of total number number of bytes copied so far, as well as "currently
copying/moving file with name X".

/ Jonas

Received on Monday, 15 July 2013 22:58:11 UTC