885753 - DeviceStorage: Change getDeviceStorage to return the default storage area.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Dave Hylands [:dhylands]]

Over in bug 874313 comment 25, Jonas made a suggestion that I would like to refine a bit further.

I'd like to propose that rather than having getDeviceStorage return a composite storage area, it should just return the storage area that corresponds with the default one (the storage area whose .default attribute would be true).

This would imply the following:

- to a get a list of all files on all storage areas, you would need to enumerate through the individual storage areas (i.e. getDeviceStorages)

- to get total free space/used space, you would need to enuerate through the individual storage areas. It would make sense that the settings app would just show the individual storage areas and not show a composite one at all.

- I still like the idea of being able to pass in /storagename/foo.txt to be able to manipulate a file from a particular storage area, so I think it makes sense to retain this behaviour (for all device storage areas rather than just the composite one). And I think we should continue to return fully qualified names. So if you call Add('foo.txt') it should still return /sdcard/foo.txt (if you were trying to add to the sdcard storage area).

Comment 1

[Evelyn Hung]

nominate leo? because it's highly related to bug 874313 which is a leo+.

Comment 2

[Dietrich Ayala (:dietrich)]

It's not clear why this needs to block 1.1. What's the user impact? Why can't these changes to in bug 874313 if they're absolutely required?

Can we fix this in 1.2 instead?

Comment 3

[Dave Hylands [:dhylands]]

Bug 874313 is for gaia work, this bug is for gecko work (which is why the two pieces of work are in separate bugs).

Comment 4

[Evelyn Hung]

(In reply to Dave Hylands [:dhylands] (PTO back July 2) from comment #3)
> Bug 874313 is for gaia work, this bug is for gecko work (which is why the
> two pieces of work are in separate bugs).
Dave, thanks for your confirm.
Yes, so we need both fixed and landed so the view will match behavior. 
Nominate again.

Comment 5

[Alex Keybl [:akeybl]]

This leo+ is conditional upon this being the lowest risk method of fixing bug 874313

Comment 6

[Dave Hylands [:dhylands]]

Attachment: Removes most of the composite storage support

This patch removes most of the support for composite device storage, and makes getDeviceStorage return the default storage area.

The remnants of composite storage which remain are that AddNamed, Delete, Get allow fully qualified storage areas to be specified.

Comment 7

[Preeti Raghunath(:Preeti)]

Any update with fix?

Comment 8

[David Flanagan [:djf]]

This bug is a month old and this is the first I've heard of it....

MediaDB still uses the composite storage object, and if I understand correctly, this patch will break Gallery, Video and Music on Leo devices or any device that has more than one device storage area.

Dave: is there a corresponding bug for fixing mediadb.js to match this change?

Comment 9

[Dave Hylands [:dhylands]]

Jan - any ETA on the review? I can find someone else if you're too busy.

Comment 10

[Jan Varga [:janv]]

I'll try to finish it tomorrow.

Comment 11

[Jan Varga [:janv]]

Comment on attachment 775032 [details] [diff] [review]
Removes most of the composite storage support

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

looks good

::: dom/devicestorage/DeviceStorage.h
@@ +167,4 @@
>                  const nsAString& aVolName);
>  
>    bool IsAvailable();
> +  bool IsFullPath(const nsAString& aPath) { return aPath.Length() > 0 && aPath.CharAt(0) == '/'; }

Nit: 80 chars per line

@@ +230,5 @@
>                                       nsDOMDeviceStorage** aStore);
>  
>    static void CreateDeviceStoragesFor(nsPIDOMWindow* aWin,
>                                        const nsAString& aType,
> +                                      nsTArray<nsRefPtr<nsDOMDeviceStorage> >& aStores);

dtto

::: dom/devicestorage/DeviceStorageRequestParent.cpp
@@ +359,5 @@
>    CopyASCIItoUTF16(mMimeType, mime);
>  
> +  nsString fullPath;
> +  mFile->GetFullPath(fullPath);
> +  nsCOMPtr<nsIDOMBlob> blob = new nsDOMFileFile(fullPath, mime, mLength, mFile->mFile, mLastModificationDate);

dtto

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +1296,5 @@
>      // TODO - needs janv's file handle support.
>      return JSVAL_NULL;
>    }
>  
> +  nsString  fullPath;

Nit: extra space

@@ +2449,5 @@
>  // static
>  void
>  nsDOMDeviceStorage::CreateDeviceStoragesFor(nsPIDOMWindow* aWin,
>                                              const nsAString &aType,
> +                                            nsTArray<nsRefPtr<nsDOMDeviceStorage> > &aStores)

Nit: 80 chars per line

@@ +2507,5 @@
>    return true;
>  }
>  
>  already_AddRefed<nsDOMDeviceStorage>
> +nsDOMDeviceStorage::GetStorage(const nsAString& aFullPath, nsAString& aOutStoragePath)

dtto

@@ +2555,1 @@
>                                             nsAString& aStorageName)

Nit: indentation

@@ +3075,5 @@
>    }
>  
>  #ifdef MOZ_WIDGET_GONK
>    else if (!strcmp(aTopic, NS_VOLUME_STATE_CHANGED)) {
> +    // We invalidate the used space cache for the volume that actually changed state.

Nit: 80 chars per line

Comment 12

[Dave Hylands [:dhylands]]

https://hg.mozilla.org/integration/b2g-inbound/rev/046e78d8a522
https://hg.mozilla.org/integration/b2g-inbound/rev/b8bb4f63c226

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/046e78d8a522
https://hg.mozilla.org/mozilla-central/rev/b8bb4f63c226

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Needs branch-specific patches for uplift.

Comment 16

[Dave Hylands [:dhylands]]

Attachment: 1.1 specific version of the patch.

Carrying r+

This patch doesn't have the whitespace changes, just the functional ones.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/bfd08abe2115