777088 - device Storage - file stat API

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Doug Turner (:dougt)]

Something like:

var storage = navigator.getDeviceStorage("testing");

request = storage.stat();

request.onsuccess = function(e) {
  e.target.result.freeBytes;
  e.target.result.totalBytes;
};

We may later want to support if it is 'connected' or mounted.

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

A couple of comments:

I think that .totalBytes should reflect the amount of usage in just the given DeviceStorage area, not total usage on the disk. (freeBytes is obviously more complicated because the free storage is shared between many DeviceStorage areas, but I think that's fine).

Since getting the totalBytes usage could be a quite expensive operation, I think it would be great if you could pass in a dictionary indicating which data you are interested in. Something like:

request = storage.stat({ totalBytes: true, freeBytes: true });

We did basically exactly that in the FileHandle API for getMetadata which is a very similar function.

We have dictionary parser for C++ in the tree these days if you want to go this route.

Comment 3

[Doug Turner (:dougt)]

this is good feedback!

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 647734 [details] [diff] [review]
patch v.1

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

::: dom/base/nsDOMClassInfo.cpp
@@ +1440,5 @@
>    NS_DEFINE_CLASSINFO_DATA(DeviceStorageChangeEvent, nsDOMGenericSH,
>                             DOM_DEFAULT_SCRIPTABLE_FLAGS)
>  
>    NS_DEFINE_CLASSINFO_DATA(DeviceStorageCursor, nsDOMGenericSH,
>                             DOM_DEFAULT_SCRIPTABLE_FLAGS)

Looks like this should have the EventTarget flags and helper.

@@ +4067,5 @@
>  
> +  DOM_CLASSINFO_MAP_BEGIN(DeviceStorageStat, nsIDOMDeviceStorageStat)
> +    DOM_CLASSINFO_MAP_ENTRY(nsIDOMDeviceStorageStat)
> +    DOM_CLASSINFO_MAP_ENTRY(nsIDOMDOMRequest)
> +    DOM_CLASSINFO_MAP_ENTRY(nsIDOMEventTarget)

This thing isn't a request, and it only implements nsIDOMDeviceStorageStat.

::: dom/devicestorage/DeviceStorageRequestChild.cpp
@@ +70,5 @@
> +    }
> +
> +    case DeviceStorageResponseValue::TStatStorageResponse:
> +    {
> +      StatStorageResponse r = aValue;

Nit: This will copy. (Admittedly not important here, but if your message was bigger it might hurt).

Do this instead:

  StatStorageResponse& r = aValue.get_StatStorageResponse();

::: dom/devicestorage/DeviceStorageRequestParent.cpp
@@ +74,5 @@
> +    {
> +      DeviceStorageStatParams p = aParams;
> +
> +      nsCOMPtr<nsIFile> f;
> +      NS_NewLocalFile(p.fullpath(), false, getter_AddRefs(f));

Can this fail?

@@ +261,5 @@
> +{
> +}
> +
> +NS_IMETHODIMP
> +DeviceStorageRequestParent::StatFileEvent::Run()

This stat code is duplicated from nsDeviceStorage, would be better to have a static function somewhere.

@@ +266,5 @@
> +{
> +  nsCOMPtr<nsIRunnable> r;
> +
> +#ifndef HAVE_SYS_STATFS_H
> +  r = new PostErrorEvent(mParent, POST_ERROR_EVENT_NOT_IMPLEMENTED);

If you throw in the child when this isn't supported (see below) then I think you shouldn't be able to get here.

@@ +271,5 @@
> +  NS_DispatchToMainThread(r);
> +  return NS_OK;
> +#else
> +  nsCString path;
> +  mFile->mFile->GetNativePath(path);

Same path troubles, see below.

@@ +433,5 @@
> +{
> +  NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> +
> +  StatStorageResponse response(mFreeBytes, mTotalBytes);
> +  unused <<  mParent->Send__delete__(mParent, response);

Hm, this isn't safe. What guarantees do you have that the child process hasn't crashed and that mParent is still a valid actor?

In general this sort of thing is hard to get right, but we need to at least file a followup to make sure we receive child crash notifications here or else the parent could crash.

::: dom/devicestorage/DeviceStorageRequestParent.h
@@ +142,5 @@
> +      PostStatResultEvent(DeviceStorageRequestParent* aParent,
> +                          PRInt64 aFreeBytes,
> +                          PRInt64 aTotalBytes);
> +      ~PostStatResultEvent();
> +      NS_IMETHOD Run();

Nit: NS_DECL_NSIRUNNABLE is better (someday soon it will add the MOZ_OVERRIDE keyword too)

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +33,5 @@
>  #include "nsCRT.h"
>  #include "mozilla/Services.h"
>  #include "nsIObserverService.h"
>  
> +#ifdef HAVE_SYS_STATFS_H

Please file a followup to add a fallback patch using nsIFile or something when this is not present.

@@ +352,5 @@
>    if (!cx) {
>      return JSVAL_NULL;
>    }
>  
>    jsval wrappedFile;

Nit: Not a file any more.

@@ +372,5 @@
> +  NS_ASSERTION(aWindow, "Null Window");
> +
> +  if (aFile->mEditable) {
> +    // TODO - needs janv's file handle support.
> +    return JSVAL_NULL;

NS_WARNING maybe?

@@ +380,5 @@
> +    return JSVAL_NULL;
> +  }
> +
> +  nsCOMPtr<nsIDOMBlob> blob = new nsDOMFileFile(aFile->mFile, aFile->mPath);
> +  return InterfaceToJsval(aWindow, blob, &NS_GET_IID(nsIDOMBlob));

The old code had nsIDOMFile, does it matter here that you're changing to nsIDOMBlob?

@@ +1020,5 @@
> +    NS_ASSERTION(!NS_IsMainThread(), "Wrong thread!");
> +    nsCOMPtr<nsIRunnable> r;
> +
> +#ifndef HAVE_SYS_STATFS_H
> +    r = new PostErrorEvent(mRequest, POST_ERROR_EVENT_NOT_IMPLEMENTED, mFile);

Talked with jonas about this and we both agree that throwing an exception at call time, rather than generating an async Error event, is a better approach here.

@@ +1026,5 @@
> +    return NS_OK;
> +#else
> +    
> +    nsCString path;
> +    mFile->mFile->GetNativePath(path);

This will certainly break on windows (where we need to always do GetPath() followed by converting utf16 to utf8).

How about on linux? I can't find any docs on what character encoding it expects. mrbkap found some random thing that seemed to suggest ASCII, but the manpage doesn't really say.

Is this really the right API for us to use? Can't we just get all this through nsIFile?

::: dom/devicestorage/nsDeviceStorage.h
@@ +108,5 @@
>  };
>  
> +class nsDOMDeviceStorageStat MOZ_FINAL
> +  : public nsIDOMDeviceStorageStat
> +{

Nit: DeviceStorageFile uses this:

  class DeviceStorageFile MOZ_FINAL : public nsISupports {

This is your code so whichever is fine but I'd be consistent.

::: dom/devicestorage/test/test_stat.html
@@ +25,5 @@
> +devicestorage_setup();
> +
> +function addSuccess(e) {
> +  ok(e.target.result.freeBytes  > 0, "free bytes should exist and be greater than zero");
> +  ok(e.target.result.totalBytes > 0, "free bytes should exist and be greater than zero");

Nit: s/free/total/

@@ +42,5 @@
> +  todo(false, "stat is not available on mac or windows yet. see bug xxxx");
> +  devicestorage_cleanup();
> +} else {
> +  var storage = navigator.getDeviceStorage("testing");
> +  ok(navigator.getDeviceStorage, "Should have getDeviceStorage");

This is a little weird, you would have thrown just above if |navigator.getDeviceStorage| is not a function. Did you mean |ok(storage, ...)| instead?

Comment 5

[Doug Turner (:dougt)]

All things fixed, but:

> This stat code is duplicated from nsDeviceStorage, would be better to have a static function somewhere.

It is just one function call to a system library.  I'll hold off on refactoring some of this for a while.


> followup to make sure we receive child crash notifications here or else the parent could crash.

Is there a state flag off of these objects that I can test for?  Shouldn't this just be baked into ipdl (somehow) such that you don't have to monitor crashes out-of-band from the object that you are interacting with....

> Please file a followup to add a fallback patch using nsIFile or something when this is not present.

Bug 779255.

> The old code had nsIDOMFile, does it matter here that you're changing to nsIDOMBlob?

Either works due to qi xpconnect magic.

> Talked with jonas about this and we both agree that throwing an exception at call time, rather than generating an async Error event, is a better approach here.

That is inconsistent with how many implementations are dealing with this.  call time exceptions should be used for programatic errors.


> Is this really the right API for us to use? Can't we just get all this through nsIFile?

Yes, this is exactly how to do it.  For references, this is the API:  http://linux.die.net/man/2/statfs



I'll put another patch up w/ a different way of calculating total space per comment 2.

Comment 6

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 650213 [details] [diff] [review]
patch v.1

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

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +374,5 @@
>      if (aType.Equals(NS_LITERAL_STRING("testing"))) {
>        dirService->Get(NS_OS_TEMP_DIR, NS_GET_IID(nsIFile), getter_AddRefs(f));
>        if (f) {
> +        f->AppendRelativeNativePath(NS_LITERAL_CSTRING("device-storage-testing"));
> +        f->Create(nsIFile::DIRECTORY_TYPE, 0777);

Unrelated, but this will fail as soon as we turn on the sandbox. Need a plan for that someday.

@@ +1637,5 @@
> +    if (mIsMounted != mounted) {
> +      mIsMounted = mounted;
> +      DispatchMountChangeEvent();
> +#ifdef DEBUG
> +      printf("There has been a change in state at sdcard.  mIsMounted == %d state=%d\n", mIsMounted, state);

Let's remove this before checkin.

Comment 8

[Doug Turner (:dougt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef06eb15d520

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ef06eb15d520