Re: Polished FileSystem API proposal

On Mon, Jul 15, 2013 at 3:57 PM, Jonas Sicking <jonas@sicking.cc> wrote:
> 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?

It's on Entry, usable by both DirectoryEntry and FileEntry.  Then
DirectoryEntry adds removeRecursively.
http://www.w3.org/TR/file-system-api/#widl-Entry-remove-void-VoidCallback-successCallback-ErrorCallback-errorCallback

> 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");

dir.move("srcfile", "dir/and/new/name");

Granted, that doesn't help you if you're holding a Directory object
and don't have a way to generate a relative path to it. But perhaps
that's not a big lack?  You can always munge paths yourself.

Obviously we went with the way you'd see as iffy.

> 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" });

In this syntax, could you say it this way?

dir.move("srcfile", { dir: "dirName". 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.

Is the idea that createFile will overwrite an existing file's
contents, even if you don't supply any, e.g. nuking a file down to 0
bytes?  That might be a bit surprising.  Would you require
MakeFileOptions.data?

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

I'm not sure why you'd ever call it this way.  Perhaps disallow it for
now?  Otherwise it might become a standard hack for some bizarre
file-based locking paradigm.

> 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.

I'd skip that one; I think the other 5 make sense, and are better than
having two different paradigms to say almost the same thing depending
on which function you call [boolean vs. string argument].  If you
decide you don't want failIfExists, that makes the list even simpler.

>> 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.

Doesn't seem all that worse than myDir.openForWriting() to me, but is
openWrite better in any way?
Sure, it doesn't sound like you're opening the directory for writing,
but that's because it doesn't make any sense at all.

>> 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".

Yup.  Or number of files copied.

Received on Monday, 15 July 2013 23:48:04 UTC