858416 - Device Storage - Create a composite interface

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Dave Hylands [:dhylands]]

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.

Comment 1

[Peter Dolanjski [:pdol]]

(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?

Comment 2

[Dave Hylands [:dhylands]]

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.

Comment 3

[[:fabrice] Fabrice Desré]

Dave, also don't forget we use deviceStorage in dom/apps/src/ to track available space when installing and updating apps.

Comment 4

[Dave Hylands [:dhylands]]

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

Comment 5

[Peter Dolanjski [:pdol]]

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

[Alex Keybl [:akeybl]]

Blocks a blocker.

Comment 7

[Dave Hylands [:dhylands]]

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

Comment 8

[Dave Hylands [:dhylands]]

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

Comment 9

[Dave Hylands [:dhylands]]

Attachment: Updated test app

I realized that I forgot to even deliver the last version of the test app. This version has been totally rewritten.

Comment 10

[Dave Hylands [:dhylands]]

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

Comment 11

[Dave Hylands [:dhylands]]

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 12

[Doug Turner (:dougt)]

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?

Comment 13

[Doug Turner (:dougt)]

Comment on attachment 736577 [details] [diff] [review]
Updated test app

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

rs

Comment 14

[Dave Hylands [:dhylands]]

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

Comment 15

[Dave Hylands [:dhylands]]

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.

Comment 16

[Dave Hylands [:dhylands]]

Attachment: 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

Comment 17

[Dave Hylands [:dhylands]]

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.

Comment 18

[Dave Hylands [:dhylands]]

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

Comment 19

[Dave Hylands [:dhylands]]

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.

Comment 20

[Dave Hylands [:dhylands]]

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

Comment 21

[Dave Hylands [:dhylands]]

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

Comment 22

[Doug Turner (:dougt)]

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.

Comment 23

[Dave Hylands [:dhylands]]

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

Comment 24

[Doug Turner (:dougt)]

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

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

fix up the comments.

Comment 25

[Dave Hylands [:dhylands]]

Attachment: Rolled up version of the patch for master (birch)

Carrying forward the r+.

This patch is for master (birch)

Comment 26

[Dave Hylands [:dhylands]]

Attachment: Rolled up version of the patch for b2g18

Carrying forward the r+

This is a rolled up version of the patch for b2g18

Comment 27

[Dave Hylands [:dhylands]]

Attachment: Rolled up version of the patch for b2g18 v2

Added the r=dougt

Comment 28

[Dave Hylands [:dhylands]]

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

Carrying forward the r+

Comment 29

[Dave Hylands [:dhylands]]

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.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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

Comment 32

[Dave Hylands [:dhylands]]

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

Comment 33

[Dave Hylands [:dhylands]]

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

Comment 34

[Dave Hylands [:dhylands]]

Attachment: Rolled up version of the patch for master (birch) v2

Fixed tests

Comment 35

[Dave Hylands [:dhylands]]

Attachment: Rolled up version of the patch for master (birch) v3

Attach the right patch

Comment 36

[Dave Hylands [:dhylands]]

Attachment: Rolled up version of the patch for b2g18 v3

Fixed tests

Comment 37

[Dave Hylands [:dhylands]]

https://hg.mozilla.org/projects/birch/rev/ba7496b49c06

Comment 38

[Ryan VanderMeulen [:RyanVM]]

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

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/b8a980062526

Comment 40

[Dave Hylands [:dhylands]]

https://github.com/mozilla-b2g/gaia/commit/fdfee79fa78b767ccf4241e9e525d5e5849a8c6f

Comment 41

[John Ford [:jhford] CET/CEST Berlin Time]

v1-train: 36ae462072f20288889d77af35ef013c498d3871

Comment 42

[Dave Hylands [:dhylands]]

For device documentation, we should document navigator.getDeviceStorage and navigator.getDeviceStorages, as well as the .storageName attribute.