Feedback on the WebStorage API

[David Bruant]

Hi,

Referring to https://wiki.mozilla.org/WebAPI/DeviceStorageAPI

I'm sympathic to having a dedicated Storage API and not just a File API :-)

void addEventListener(DOMString type, EventListener listener, optional
boolean useCapture);
void removeEventListener(DOMString type, EventListener listener);
boolean dispatchEvent(DeviceStorageChangeEvent event);
=> This part can be removed by making DeviceStorage inherit from EventTarget
I see the idl file does it.

I don't understand the get/getEditable divide. getEditable is enough, no?

Both the enumerate and enumerateEditable could return ES6 generators [1]
instead of a custom thing that mimics the exact same behavior.
SpiderMonkey already has generators. If WedIDL has nothing to use ES
generators, let's send feedback to get it.

About the DeviceStorageChangeEvent, what does path refer to? The notion
of "absolute path" is hard to understand given that the Storage API is
not a File API. If it was the absolute path of the underlying
implementation, I think it'd be a security leak.

David

[1] http://wiki.ecmascript.org/doku.php?id=harmony:generators

[David Bruant]

Le 06/11/2012 18:36, David Bruant a écrit :
> Both the enumerate and enumerateEditable could return ES6 generators
> [1] instead of a custom thing that mimics the exact same behavior.
> SpiderMonkey already has generators. If WedIDL has nothing to use ES
> generators, let's send feedback to get it.

For that part, it's been brought to my attention [1] that DeviceStorage
is asynchronous, so an ES6 iterator (it's the interface. generators are
an implementation of this interface and other things) isn't the right fit.

I guess I don't understand the point of the "continue". Apparently,
there is an equivalent construct in IDB. I've seen code samples and the
weird part is the .continue. Basically, the developer has to opt-in to
continue enumerate results. It goes against the habit (and what's
natural in my opinion) to continue by default and having an opt-in for
*stopping* the enumeration:
* ES5 array extras enumerate all by default (you can throw to stop),
* iterators enumerate all by default (you have to throw a specific value
to stop the iteration)
* for/for-in loops enumerate all by default and you have to "break" to
stop the iteration

Is there a specific reason to have chosen the opposite direction for
DeviceStorageCursor? Does the web dev wants to stop the enumeration by
default?
If not, an "abort" function would be more relevant than a "continue"
function.

An alternative is to return an array of DOMRequest (or promises), each
corresponding to a given "file". With ES6 proxies (or C++ magic),
everything, even the DOMRequests instances can be built lazily.

David

[1]
http://lists.w3.org/Archives/Public/public-script-coord/2012OctDec/0130.html

[Ian Bicking]

On Wed, Nov 7, 2012 at 3:33 AM, David Bruant <brua...@gmail.com> wrote:

> Le 06/11/2012 18:36, David Bruant a écrit :
>
> Both the enumerate and enumerateEditable could return ES6 generators [1]
>> instead of a custom thing that mimics the exact same behavior. SpiderMonkey
>> already has generators. If WedIDL has nothing to use ES generators, let's
>> send feedback to get it.
>>
> For that part, it's been brought to my attention [1] that DeviceStorage is
> asynchronous, so an ES6 iterator (it's the interface. generators are an
> implementation of this interface and other things) isn't the right fit.
>
> I guess I don't understand the point of the "continue". Apparently, there
> is an equivalent construct in IDB. I've seen code samples and the weird
> part is the .continue. Basically, the developer has to opt-in to continue
> enumerate results.

I think this is because you might need to do other asynchronous things
before getting the next result. Like:

storage = navigator.getDeviceStorage("images");
cursor = storage.enumerate(".");
cursor.onsuccess = function () {
var file = cursor.result;
req = new XMLHttpRequest();
req.open("POST", "/some-service");
req.onreadystatechange = function () {
if (req.readyState == 4) {
cursor.continue();
}
};
var reader = new FileReader();
reader.onload = function () {
req.send(reader.result);
};
reader.readAsArrayBuffer(file);
};

Once you start adding error handling and other stuff the logic could get
pretty complicated, and .continue() makes it more doable.

[David Bruant]

I see. The problem with enumerate is that it returns a DOMRequest. In
all other places, DOMRequest refers to a single request; the onsuccess
callback is called only once.
For enumerate, the story is different. Enumerate returns somethings
where the onsuccess is called for each element. So while you could be
tempted to use onsuccess as you used to with other APIs to do async
work, this is a complete mistake to do that for the return value of
enumerate.

Enumerate should rather return something like {oneach, onerror}. Also,
if it did return an array of DOMRequests (or a proxy to around an array
of DOMRequests) as I suggested, there would be no problem whatsoever;
each file handling would be separated from the enumeration.
Indeed, the main problem with enumerate is that it forces onsuccess
callbacks to deal with both file handling (do something wit
cursor.result) and enumeration (choose to .continue or not... which we
can expect people to forget and have bugs with).

David

[Jonas Sicking]

On Tue, Nov 6, 2012 at 9:36 AM, David Bruant <brua...@gmail.com> wrote:
> Hi,
>
> Referring to https://wiki.mozilla.org/WebAPI/DeviceStorageAPI
>
> I'm sympathic to having a dedicated Storage API and not just a File API :-)
>
> void addEventListener(DOMString type, EventListener listener, optional
> boolean useCapture);
> void removeEventListener(DOMString type, EventListener listener);
> boolean dispatchEvent(DeviceStorageChangeEvent event);
> => This part can be removed by making DeviceStorage inherit from EventTarget
> I see the idl file does it.

This is just a documentation quirk.

> I don't understand the get/getEditable divide. getEditable is enough, no?

If all you want to do is to read from a file then get is significantly
simpler. getEditable requires two nested asynchronous function calls
before you even have a reference to a Blob object which you can read
from.

> About the DeviceStorageChangeEvent, what does path refer to? The notion of
> "absolute path" is hard to understand given that the Storage API is not a
> File API. If it was the absolute path of the underlying implementation, I
> think it'd be a security leak.

Actually, DeviceStorage is effectively a filesystem API. With some
additional top-level API to get a reference to a filesystem rooted in
your pictures folder.

/ Jonas

[David Bruant]

I see. The intermingling of other async operations and how the
enumeration should proceed looks like a footgun to me.
Unlike other APIs, enumerate shouldn't return a DOMRequest. In all other

places, DOMRequest refers to a single request; the onsuccess callback is
called only once. For enumerate, the story is different. Enumerate
returns somethings where the onsuccess is called for each element. So
while you could be tempted to use onsuccess as you used to with other

APIs to do async work, this is a mistake to do that for the return value
of enumerate.
Enumerate should rather return something like {oneach, onerror}. oneach
would make very clear that your callback is meant to be called for each
file and that someone else (the system in that case) is taking care of
the enumeration part so you don't have to worry about it. Very much like
Array.prototype.forEach. What you care about is doing something with the
different files. You don't care about how the enumeration occurs and you
should only care if you plan to abort it (which is not the 80% case in
my opinion)

David

[David Bruant]

Le 09/11/2012 08:53, Jonas Sicking a écrit :
> On Tue, Nov 6, 2012 at 9:36 AM, David Bruant <brua...@gmail.com> wrote:
>> I don't understand the get/getEditable divide. getEditable is enough, no?
> If all you want to do is to read from a file then get is significantly
> simpler. getEditable requires two nested asynchronous function calls
> before you even have a reference to a Blob object which you can read
> from.

Why would it be the case? Is there any benefit in this indirection for
getEditable?

>> About the DeviceStorageChangeEvent, what does path refer to? The notion of
>> "absolute path" is hard to understand given that the Storage API is not a
>> File API. If it was the absolute path of the underlying implementation, I
>> think it'd be a security leak.
> Actually, DeviceStorage is effectively a filesystem API. With some
> additional top-level API to get a reference to a filesystem rooted in
> your pictures folder.

When I look at the entire API, only the DeviceStorageChangeEvent
interface manipulates an absolute path; all other functions manipulate
"name" and "directory". How does one use DeviceStorageChangeEvent's path
to relate to the things manipulated by
.get/getEditable/delete/enumerate... ?

David

[David Bruant]

Hi,

Another small piece of feedback:
add and addNames just ought to be the same method with an optional argument.

David

[David Bruant]

I guess I have another one. The wikimo page says that addNamed will fail
if a file of this name already exists, but there is no way to check if a
name exists. So it means that currently, if one wants to check if a name
is in the storage already, this person has to pull out the entire file
through a "get" call to check.
It might be worth considering adding a .has method

David