Device Storage - Create a composite interface

RESOLVED FIXED in Firefox 23

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: dhylands, Assigned: dhylands)

Tracking

({dev-doc-needed})

unspecified
mozilla23
ARM
Gonk (Firefox OS)
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-moztrap -

Firefox Tracking Flags

(blocking-b2g:leo+, firefox21 wontfix, firefox22 wontfix, firefox23 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

Details

(Whiteboard: [fixed-in-birch])

Attachments

(3 attachments, 9 obsolete attachments)

21.46 KB, patch
dougt
: review+
Details | Diff | Splinter Review
131.36 KB, patch
dhylands
: review+
Details | Diff | Splinter Review
128.48 KB, patch
dhylands
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Now that we have getDeviceStorages returning an array of device storage objects, it makes sense to have getDeviceStorage return an object which represents the union of the objects returned by getDveviceStorages.

For gonk, the volume name would be prepended to filename to allow the composite device storage object to know which of the sub-objects the file actually belongs to.

So a filename like sdcard:somedir/somefile.jpg would be associated with the sdcard device storage object and would be somedir/somefile.jpg within that object.

For non-gonk platforms (right now, only gonk platforms support volumes), no composite storage object will be created, and no volume names will be expected or looked for.

Having this composite device storage object will dramatically simplify supporting multiple volumes for FirefoxOS.
(Assignee)

Updated

5 years ago
Assignee: nobody → dhylands
(In reply to Dave Hylands [:dhylands] from comment #0)
> 
> Having this composite device storage object will dramatically simplify
> supporting multiple volumes for FirefoxOS.

Dave, could you elaborate on this a little bit?  Who benefits particularly and do you believe we should block the release if not implemented?
Flags: needinfo?(dhylands)
(Assignee)

Comment 2

5 years ago
So I took a look at what gaia changes would be needed to support multiple device storage objects. I found about 60 call sites.

With the composite device storage object it means that about 50 of those would remain untouched.  Without the composite device storage object, every single one of those call sites need to be changed, and many of them in a non-trivial manner. I doubt it could be done in time (I think that this is why djf said it would take at least a month to implement bug 838038)

With the composite device storage object, the places which need to be changed should just need some minor changes. In particular when saving a new file, the caller may need to specify which volume to save it on.
Flags: needinfo?(dhylands)
Dave, also don't forget we use deviceStorage in dom/apps/src/ to track available space when installing and updating apps.
(Assignee)

Comment 4

5 years ago
(In reply to Fabrice Desré [:fabrice] from comment #3)
> Dave, also don't forget we use deviceStorage in dom/apps/src/ to track
> available space when installing and updating apps.

Yep - my plan for that is that for this case (apps) you'll just the original device storage object that you got before - no composite object is required.

The composite object is only required for the media storage portion where multiple volumes may be present.
Based on Dave's comment regarding the large number of changes to make 838038 work properly, I think we should block on this.

Comment 6

5 years ago
Blocks a blocker.
blocking-b2g: leo? → leo+
(Assignee)

Comment 7

5 years ago
Created attachment 736509 [details] [diff] [review]
Add a composite DeviceStorage object

This adds support for a composite device storage object, which front-ends the multiple device storage objects returned from navigator.getDeviceStorages.

I also opted to change the volumeName field in the nsIDeviceStorage interface to be storageName, because I think storageName is a better name.

There is still some work needed in bluetooth file transfer and camera to be able to change the storage location based on a setting, but for now, I just made them continue to save to sdcard.

I tested Gallery with the new composite interface, and it seems to work with no changes.
(Assignee)

Updated

5 years ago
Attachment #736509 - Flags: review?(doug.turner)
(Assignee)

Comment 9

5 years ago
Created attachment 736577 [details] [diff] [review]
Updated test app

I realized that I forgot to even deliver the last version of the test app. This version has been totally rewritten.
Attachment #736577 - Flags: review?(doug.turner)
(Assignee)

Comment 10

5 years ago
Created attachment 737025 [details] [diff] [review]
Some fixups from try run failures.

https://tbpl.mozilla.org/?tree=Try&rev=fd24e36ecf37

The try run before this uncovered some errors, that are fixed in this patchset, which should be greener.
Attachment #736509 - Attachment is obsolete: true
Attachment #736509 - Flags: review?(doug.turner)
Attachment #737025 - Flags: review?(doug.turner)
(Assignee)

Comment 11

5 years ago
This is the try run I meant to post. It looks like all of the device storage tests are passing now.
https://tbpl.mozilla.org/?tree=Try&rev=8609d7db1135
Comment on attachment 737025 [details] [diff] [review]
Some fixups from try run failures.

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

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +556,1 @@
>        } else {

Why is the default sdcard?  Should this be decided in DeviceStorageFile() -- maybe it be optional.

::: dom/devicestorage/DeviceStorageCompositePath.h
@@ +14,5 @@
> +  // storageName:path
> +  // This helper class can take either a composite path, or
> +  // a storageName and storage-relative path and convert between
> +  // the 2 forms.
> +  DeviceStorageCompositePath() {}

: is a valid character in a file path.

localhost:~ dougt$ /bin/bash
[dougt ~] $ mkdir a\:a
[dougt ~] $ ls
Applications	Documents	Dropbox		Movies		Pictures	Sites		builds
Desktop		Downloads	Library		Music		Public		a:a

Is there some other char we should use?

@@ +29,5 @@
> +    nsString compositePath;
> +    CompositePath(compositePath);
> +    return compositePath;
> +  }
> +  void CompositePath(nsAString& outCompositePath);

I think I would rather just have this method and not the one that returns a concrete class.

@@ +39,5 @@
> +    mStorageName = aStorageName;
> +    mStoragePath = aStoragePath;
> +  }
> +
> +  nsString DefaultStorageName();

out reference param

::: dom/devicestorage/DeviceStorageRequestChild.cpp
@@ +105,5 @@
>          if (NS_FAILED(rv)) {
>            continue;
>          }
>  
> +        DeviceStorageCompositePath  compositePath(r.paths()[i].name());

two spaces. :(

@@ +108,5 @@
>  
> +        DeviceStorageCompositePath  compositePath(r.paths()[i].name());
> +        nsRefPtr<DeviceStorageFile> dsf = new DeviceStorageFile(r.paths()[i].type(),
> +                                                                compositePath.StorageName(),
> +                                                                f);

The white space might be really wrong here.. not sure.

::: dom/devicestorage/DeviceStorageRequestParent.cpp
@@ +31,5 @@
>  {
>    MOZ_COUNT_CTOR(DeviceStorageRequestParent);
>  }
>  
> +nsString DeviceStorageRequestParent::GetStorageName(const nsAString& aPath)

Don't return nsString.  (could be wrong about this)  Instead use a ref?

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +281,1 @@
>                                                             fullpath,

white space?

@@ +319,3 @@
>                                       nsIFile* aFile,
>                                       const nsAString& aPath)
> +  : mStorageName(aStorageName)

Make sure the order here matches the order in the declaration so that we don't add warnings.

@@ +568,5 @@
> +  nsTArray<nsString>::index_type i;
> +  for (i = 0; i < n; i++) {
> +    nsCOMPtr<nsIVolume> vol;
> +    rv = vs->GetVolumeByName(volNames[i], getter_AddRefs(vol));
> +    NS_ENSURE_SUCCESS_VOID(rv);

just continue?

@@ +571,5 @@
> +    rv = vs->GetVolumeByName(volNames[i], getter_AddRefs(vol));
> +    NS_ENSURE_SUCCESS_VOID(rv);
> +    int32_t volState;
> +    rv = vol->GetState(&volState);
> +    NS_ENSURE_SUCCESS_VOID(rv);

continue;  and same for the rest...

@@ +579,5 @@
> +    nsString volMountPoint;
> +    rv = vol->GetMountPoint(volMountPoint);
> +    NS_ENSURE_SUCCESS_VOID(rv);
> +    mStorageName = volNames[i];
> +    NS_NewLocalFile(volMountPoint, false, getter_AddRefs(mFile));

this can fail..  test for rv, or mFile

@@ +580,5 @@
> +    rv = vol->GetMountPoint(volMountPoint);
> +    NS_ENSURE_SUCCESS_VOID(rv);
> +    mStorageName = volNames[i];
> +    NS_NewLocalFile(volMountPoint, false, getter_AddRefs(mFile));
> +    mFile->AppendRelativePath(mPath);

Test for failure.

@@ +585,5 @@
> +    rv = mFile->GetPath(rootPath);
> +    NS_ENSURE_SUCCESS_VOID(rv);
> +    collectFilesInternal(aFiles, aSince, rootPath);
> +    mStorageName = NS_LITERAL_STRING("");
> +    mFile = nullptr;

I don't understand why you are changing these two variables here.

@@ +1871,5 @@
> +{
> +public:
> +  CompositePathHelper(const nsAString& aCompositePath,
> +                      const nsTArray<nsRefPtr<nsDOMDeviceStorage> >& aStores);
> +  nsresult Result() { return mResult; }

how about a boolean instead of using error codes?  I am not sure if using error codes here is the right thing.

@@ +1873,5 @@
> +  CompositePathHelper(const nsAString& aCompositePath,
> +                      const nsTArray<nsRefPtr<nsDOMDeviceStorage> >& aStores);
> +  nsresult Result() { return mResult; }
> +  const nsAString& StoragePath() { return mCompositePath.StoragePath(); }
> +  nsDOMDeviceStorage *Storage() { return mStorage.get(); }

returning an non-refcounted pointer will cause you pain later.

@@ +2124,5 @@
> +    nsString volMountPoint;
> +    rv = vol->GetMountPoint(volMountPoint);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    nsCOMPtr<nsIFile> f;
> +    NS_NewLocalFile(volMountPoint, false, getter_AddRefs(f));

This code block looks very similar to the one above.  Does it make sense to pull this into some function that enumerates and calls some callback for all mountpoints?
Attachment #737025 - Flags: review?(doug.turner) → review-
Comment on attachment 736577 [details] [diff] [review]
Updated test app

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

rs
Attachment #736577 - Flags: review?(doug.turner) → review+
(Assignee)

Comment 14

5 years ago
(In reply to Doug Turner (:dougt) from comment #12)
> Comment on attachment 737025 [details] [diff] [review]
> Some fixups from try run failures.
> 
> Review of attachment 737025 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/BluetoothOppManager.cpp
> @@ +556,1 @@
> >        } else {
> 
> Why is the default sdcard?  Should this be decided in DeviceStorageFile() --
> maybe it be optional.

This was just to make things the way they were. Bug 856782 will make it choose based on a setting.

> ::: dom/devicestorage/DeviceStorageCompositePath.h
> @@ +14,5 @@
> > +  // storageName:path
> > +  // This helper class can take either a composite path, or
> > +  // a storageName and storage-relative path and convert between
> > +  // the 2 forms.
> > +  DeviceStorageCompositePath() {}
> 
> : is a valid character in a file path.

: isn't valid in FAT formatted partitions (which all sdcard volumes are).
In regular linux filesystems all characters are legal except / and null, so then I think I'd have to propose something like:

/volume/dir/file

(i.e. any pathname that starts with a slash is assumed to be /volume/ and dir/file would be relative to the mount location of the volume.


> @@ +29,5 @@
> > +    nsString compositePath;
> > +    CompositePath(compositePath);
> > +    return compositePath;
> > +  }
> > +  void CompositePath(nsAString& outCompositePath);
> 
> I think I would rather just have this method and not the one that returns a
> concrete class.

I was just trying to reduce a few lines of code in a couple of places.

> two spaces. :(

I think I got some hard-tabs in there somewhere - I'll clean them up.

> > +nsString DeviceStorageRequestParent::GetStorageName(const nsAString& aPath)
> 
> Don't return nsString.  (could be wrong about this)  Instead use a ref?

You definitely don't want to return a ref or ptr. nsString isn't ref counted so you'd have to return by out param if you don't like the return by object.

> >                                       nsIFile* aFile,
> >                                       const nsAString& aPath)
> > +  : mStorageName(aStorageName)
> 
> Make sure the order here matches the order in the declaration so that we
> don't add warnings.

It's supposed to mach the order of that the variables are declared in the class, which they do (no necessarily the order of the arguments to the constructor).

> @@ +568,5 @@
> > +  nsTArray<nsString>::index_type i;
> > +  for (i = 0; i < n; i++) {
> > +    nsCOMPtr<nsIVolume> vol;
> > +    rv = vs->GetVolumeByName(volNames[i], getter_AddRefs(vol));
> > +    NS_ENSURE_SUCCESS_VOID(rv);
> 
> just continue?

You can't. If you get an error, that means that the volume doesn't exist. That should be an error.

> 
> @@ +571,5 @@
> > +    rv = vs->GetVolumeByName(volNames[i], getter_AddRefs(vol));
> > +    NS_ENSURE_SUCCESS_VOID(rv);
> > +    int32_t volState;
> > +    rv = vol->GetState(&volState);
> > +    NS_ENSURE_SUCCESS_VOID(rv);
> 
> continue;  and same for the rest...

I don't understand why you would want to continue if you get an error.

> @@ +579,5 @@
> > +    nsString volMountPoint;
> > +    rv = vol->GetMountPoint(volMountPoint);
> > +    NS_ENSURE_SUCCESS_VOID(rv);
> > +    mStorageName = volNames[i];
> > +    NS_NewLocalFile(volMountPoint, false, getter_AddRefs(mFile));
> 
> this can fail..  test for rv, or mFile

You seem to be contradicting yourself, telling me to test for failure here, and ignore the failures earlier.

> @@ +585,5 @@
> > +    rv = mFile->GetPath(rootPath);
> > +    NS_ENSURE_SUCCESS_VOID(rv);
> > +    collectFilesInternal(aFiles, aSince, rootPath);
> > +    mStorageName = NS_LITERAL_STRING("");
> > +    mFile = nullptr;
> 
> I don't understand why you are changing these two variables here.

collectFilesInternal needs them to be set. Actually, the right thing to do is to lookup the correct device storage object based on the volume name and call that object->collectFilesInternal.

> @@ +1871,5 @@
> > +{
> > +public:
> > +  CompositePathHelper(const nsAString& aCompositePath,
> > +                      const nsTArray<nsRefPtr<nsDOMDeviceStorage> >& aStores);
> > +  nsresult Result() { return mResult; }
> 
> how about a boolean instead of using error codes?  I am not sure if using
> error codes here is the right thing.

I suppose I could translate the error to a boolean and then return some arbitrary error if I detect the boolean. I was just making it return whatever error was encountered. Seems more useful than tranlating all errors into a single error code.

> @@ +1873,5 @@
> > +  CompositePathHelper(const nsAString& aCompositePath,
> > +                      const nsTArray<nsRefPtr<nsDOMDeviceStorage> >& aStores);
> > +  nsresult Result() { return mResult; }
> > +  const nsAString& StoragePath() { return mCompositePath.StoragePath(); }
> > +  nsDOMDeviceStorage *Storage() { return mStorage.get(); }
> 
> returning an non-refcounted pointer will cause you pain later.

I guess I can return a TemporaryRef<> I was told returning RefPtr's was bad.

> @@ +2124,5 @@
> > +    nsString volMountPoint;
> > +    rv = vol->GetMountPoint(volMountPoint);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +    nsCOMPtr<nsIFile> f;
> > +    NS_NewLocalFile(volMountPoint, false, getter_AddRefs(f));
> 
> This code block looks very similar to the one above.  Does it make sense to
> pull this into some function that enumerates and calls some callback for all
> mountpoints?

The callbacks all need to take different args. I might be able to create an iterator or something. Although it seems like a lot of work just to cover off 2 cases.

I could create a VolumeIterator as well I suppose. Althogh again, this feels more like "make work".
(Assignee)

Updated

5 years ago
Blocks: 862784
(Assignee)

Comment 15

5 years ago
I've been delayed working on bug 860934, so this bug won't quite be ready for the end of today. I need to rebase on top of bug 860934, re-test, and re-review.
Whiteboard: [ETA Apr 26]
(Assignee)

Comment 16

5 years ago
Created attachment 743353 [details] [diff] [review]
Add a composite DeviceStorage object. v3

Rebased ontop of bug 860934 (security bug).

Essentially, I had to rewrite alsmost the entire patch, but a bunch of stuff got cleaned up due to the way things are after bug 860934 landed.

This patch now specifies device storage paths as:

/storageName/dir/file

Note that, while this looks like a path, you need to translate storageName into a mount point (so storageName sdcard has a mount point of /mnt/sdcard).

So if you were to do:

storage.enumerate('prefix')

you'll now get back something like:

/storageName/prefix/dir/file

If you're on a non-volume based system, like unix or windows, then you'll get back:

prefix/dir/file
Attachment #737025 - Attachment is obsolete: true
(Assignee)

Comment 17

5 years ago
Comment on attachment 743353 [details] [diff] [review]
Add a composite DeviceStorage object. v3

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=ac917c69ea14

Some further changes will be required once bug 853732 lands.
Attachment #743353 - Flags: review?(doug.turner)
(Assignee)

Comment 18

5 years ago
Created attachment 744324 [details] [diff] [review]
Make saving new files be per pref

This adds support for saving files to a user-selected volume (part of bug 856782).

When using a composite device along with Add or AddNamed, if no storage location is specified, then it will first try the storage name specified in the device.storage.writable.name.
If that storage name isn't available, then the first available storage location will be chosen (the list is ordered such that sdcard will be first, followed by other storage locations).

The request object returned by Add and AddNamed will have the actual storage location returned in the e.target.result as part of the onsuccess callback.
Attachment #744324 - Flags: review?(doug.turner)
(Assignee)

Comment 19

5 years ago
Comment on attachment 744324 [details] [diff] [review]
Make saving new files be per pref

I realized that I need to fix bluetooth and camera to fit into this logic somehow, so more work is needed.
Attachment #744324 - Flags: review?(doug.turner)
(Assignee)

Comment 20

5 years ago
Created attachment 747732 [details] [diff] [review]
Make saving new files be per pref. v2

This patchset rebases ontop of bug 853732 and makes the used-space cache be per-volume.
It also makes saving new files which don't specify a storageName use the device.storage.writable.name pref (which is mirrored from the setting of the same name).

Add/AddNamed/Get/GetEditable/Delete all return the fully qualified storage name of where the file was retrieved from, if a storageName was not specified.

Enumerate also now returns fully qualified storage names.
Attachment #744324 - Attachment is obsolete: true
Attachment #747732 - Flags: review?(doug.turner)
Comment on attachment 743353 [details] [diff] [review]
Add a composite DeviceStorage object. v3

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

::: dom/base/Navigator.cpp
@@ +982,4 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP Navigator::GetDeviceStorages(const nsAString &aType, nsIVariant** _retval)

are we still keeping both methods?

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +576,5 @@
>      nsresult rv = mimeSvc->GetTypeFromFile(f, mimeType);
>  
>      if (NS_SUCCEEDED(rv)) {
> +      // The hard-coded sdcard references will be replaced as part of
> +      // bug 856782

This bug looks like it is fixed?

@@ +585,2 @@
>        } else if (StringBeginsWith(mimeType, NS_LITERAL_CSTRING("video/"))) {
> +        mDsFile = new DeviceStorageFile(NS_LITERAL_STRING("movies"),

Shouldn't this be videos?

::: dom/devicestorage/DeviceStorageRequestParent.cpp
@@ +600,5 @@
>  {
>    NS_ASSERTION(!NS_IsMainThread(), "Wrong thread!");
>  
>    nsCOMPtr<nsIRunnable> r;
> +  if (mFile->mFile) {

early return?

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +68,5 @@
>  #include "nsDirectoryServiceDefs.h"
>  
> +// Helper macro for iterating through the volumes associated with a
> +// composite device.
> +#define FOR_EACH_VOLUME(volNames, volIdx) \

I am very opposed to macros.

@@ +72,5 @@
> +#define FOR_EACH_VOLUME(volNames, volIdx) \
> +  nsTArray<nsString> volNames; \
> +  nsDOMDeviceStorage::GetOrderedVolumeNames(volNames); \
> +  nsTArray<nsString>::size_type numVolumes = volNames.Length(); \
> +  nsTArray<nsString>::index_type volIdx; \

you probably can just use an int32_t inside the for loop?

@@ +394,5 @@
> +  } else {
> +    ptStr = "child";
> +  }
> +
> +  printf_stderr("DSF (%s) %s: mStorageType '%s' mStorageName '%s' mRootDir '%s' mPath '%s' mFile->GetPath '%s'\n",

Does this work cross platform?  I am not sure if printf_stderr() is defined everywhere.

::: dom/ipc/PContent.ipdl
@@ +59,5 @@
>      uint8_t   italic;
>      uint8_t   index;
>  };
>  
>  struct DeviceStorageFreeSpaceParams

we probably want to factor type and storageName out into a separate type at some point.

@@ +491,5 @@
>                                      bool aElementHidden);
>  
>      async AudioChannelChangedNotification();
>  
> +    async FilePathUpdateNotify(nsString aStorageType,

use aType -- it is consistent with the other variable names used here.
(Assignee)

Comment 23

5 years ago
(In reply to Doug Turner (:dougt) from comment #22)
> Comment on attachment 743353 [details] [diff] [review]
> Add a composite DeviceStorage object. v3
> 
> Review of attachment 743353 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/Navigator.cpp
> @@ +982,4 @@
> >    return NS_OK;
> >  }
> >  
> >  NS_IMETHODIMP Navigator::GetDeviceStorages(const nsAString &aType, nsIVariant** _retval)
> 
> are we still keeping both methods?

Yes. GetDeviceStorage returns the composite interface. GetDeviceStorages returns the array of non-composite storage areas. These are used in the settings app for showing free/used space on a per-volume basis.

> ::: dom/bluetooth/BluetoothOppManager.cpp
> @@ +576,5 @@
> >      nsresult rv = mimeSvc->GetTypeFromFile(f, mimeType);
> >  
> >      if (NS_SUCCEEDED(rv)) {
> > +      // The hard-coded sdcard references will be replaced as part of
> > +      // bug 856782
> 
> This bug looks like it is fixed?

Yep - I'll fix the comment.

> @@ +585,2 @@
> >        } else if (StringBeginsWith(mimeType, NS_LITERAL_CSTRING("video/"))) {
> > +        mDsFile = new DeviceStorageFile(NS_LITERAL_STRING("movies"),
> 
> Shouldn't this be videos?

Yes - it should.

> ::: dom/devicestorage/DeviceStorageRequestParent.cpp
> @@ +600,5 @@
> >  {
> >    NS_ASSERTION(!NS_IsMainThread(), "Wrong thread!");
> >  
> >    nsCOMPtr<nsIRunnable> r;
> > +  if (mFile->mFile) {
> 
> early return?
> 
> ::: dom/devicestorage/nsDeviceStorage.cpp
> @@ +68,5 @@
> >  #include "nsDirectoryServiceDefs.h"
> >  
> > +// Helper macro for iterating through the volumes associated with a
> > +// composite device.
> > +#define FOR_EACH_VOLUME(volNames, volIdx) \
> 
> I am very opposed to macros.

ok - I'll copy and paste it out long hand. I created the macro due to some comments you made in an earlier review.

> @@ +72,5 @@
> > +#define FOR_EACH_VOLUME(volNames, volIdx) \
> > +  nsTArray<nsString> volNames; \
> > +  nsDOMDeviceStorage::GetOrderedVolumeNames(volNames); \
> > +  nsTArray<nsString>::size_type numVolumes = volNames.Length(); \
> > +  nsTArray<nsString>::index_type volIdx; \
> 
> you probably can just use an int32_t inside the for loop?

No - it needs to be unsigned. Using index_type and size_type is the "right" way. I can create a typedef like:

typedef nsTArray<nsString> VolumeNameArray;

and then using VolumeNameArray::index_type;

> @@ +394,5 @@
> > +  } else {
> > +    ptStr = "child";
> > +  }
> > +
> > +  printf_stderr("DSF (%s) %s: mStorageType '%s' mStorageName '%s' mRootDir '%s' mPath '%s' mFile->GetPath '%s'\n",
> 
> Does this work cross platform?  I am not sure if printf_stderr() is defined
> everywhere.

Yes it is defined everywhere. See green try run.

> ::: dom/ipc/PContent.ipdl
> @@ +59,5 @@
> >      uint8_t   italic;
> >      uint8_t   index;
> >  };
> >  
> >  struct DeviceStorageFreeSpaceParams
> 
> we probably want to factor type and storageName out into a separate type at
> some point.

Yeah - we could create a tuple, since you always need both. I really don't want to do that as part of this bug. I'm happy to file a followup bug.

Updated

5 years ago
Attachment #747732 - Flags: review?(doug.turner) → review+
Comment on attachment 743353 [details] [diff] [review]
Add a composite DeviceStorage object. v3

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

fix up the comments.
Attachment #743353 - Flags: review?(doug.turner) → review+
(Assignee)

Comment 25

5 years ago
Created attachment 748402 [details] [diff] [review]
Rolled up version of the patch for master (birch)

Carrying forward the r+.

This patch is for master (birch)
Attachment #743353 - Attachment is obsolete: true
Attachment #747732 - Attachment is obsolete: true
Attachment #748402 - Flags: review+
(Assignee)

Comment 26

5 years ago
Created attachment 748403 [details] [diff] [review]
Rolled up version of the patch for b2g18

Carrying forward the r+

This is a rolled up version of the patch for b2g18
Attachment #748403 - Flags: review+
(Assignee)

Comment 27

5 years ago
Created attachment 748404 [details] [diff] [review]
Rolled up version of the patch for b2g18 v2

Added the r=dougt
Attachment #748403 - Attachment is obsolete: true
(Assignee)

Comment 28

5 years ago
Comment on attachment 748404 [details] [diff] [review]
Rolled up version of the patch for b2g18 v2

Carrying forward the r+
Attachment #748404 - Flags: review+
(Assignee)

Comment 29

5 years ago
https://hg.mozilla.org/projects/birch/rev/dbf0c9f4a4b9

Note that there is a b2g18 specific version of the patch attched for uplift to the b2g18 stream.
Whiteboard: [fixed-in-birch]
https://hg.mozilla.org/mozilla-central/rev/dbf0c9f4a4b9
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Backed out for B2G mochitest-3 failures that I didn't notice before merging to m-c.
https://hg.mozilla.org/mozilla-central/rev/af2811479de4

https://tbpl.mozilla.org/php/getParsedLog.php?id=22860764&tree=Birch
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla23 → ---
(Assignee)

Comment 34

5 years ago
Created attachment 748510 [details] [diff] [review]
Rolled up version of the patch for master (birch) v2

Fixed tests
Attachment #748402 - Attachment is obsolete: true
Attachment #748510 - Flags: review+
(Assignee)

Comment 35

5 years ago
Created attachment 748511 [details] [diff] [review]
Rolled up version of the patch for master (birch) v3

Attach the right patch
Attachment #748510 - Attachment is obsolete: true
Attachment #748511 - Flags: review+
(Assignee)

Comment 36

5 years ago
Created attachment 748512 [details] [diff] [review]
Rolled up version of the patch for b2g18 v3

Fixed tests
Attachment #748404 - Attachment is obsolete: true
Attachment #748512 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ba7496b49c06
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
https://hg.mozilla.org/releases/mozilla-b2g18/rev/b8a980062526
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-firefox21: --- → wontfix
status-firefox22: --- → wontfix
status-firefox23: --- → fixed
v1-train: 36ae462072f20288889d77af35ef013c498d3871

Updated

5 years ago
Depends on: 871956
(Assignee)

Updated

5 years ago
No longer blocks: 872238
Depends on: 872238
(Assignee)

Updated

5 years ago
Depends on: 872800
Depends on: 873065
(Assignee)

Updated

5 years ago
Depends on: 872170
(Assignee)

Updated

5 years ago
Depends on: 875372
(Assignee)

Updated

5 years ago
Depends on: 875637
(Assignee)

Updated

5 years ago
Depends on: 876055
Depends on: 877087

Updated

5 years ago
Flags: in-moztrap-
(Assignee)

Updated

4 years ago
Keywords: dev-doc-needed
(Assignee)

Comment 42

4 years ago
For device documentation, we should document navigator.getDeviceStorage and navigator.getDeviceStorages, as well as the .storageName attribute.
You need to log in before you can comment on or make changes to this bug.