Closed Bug 885753 Opened 6 years ago Closed 6 years ago

DeviceStorage: Change getDeviceStorage to return the default storage area.

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26
blocking-b2g leo+
Tracking Status
firefox24 --- wontfix
firefox25 --- wontfix
firefox26 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: dhylands, Assigned: dhylands)

References

Details

Attachments

(2 files)

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).
nominate leo? because it's highly related to bug 874313 which is a leo+.
blocking-b2g: --- → leo?
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?
blocking-b2g: leo? → -
Bug 874313 is for gaia work, this bug is for gecko work (which is why the two pieces of work are in separate bugs).
(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.
blocking-b2g: - → leo?
Blocks: 874313
blocking-b2g: leo? → leo+
No longer depends on: 874313
This leo+ is conditional upon this being the lowest risk method of fixing bug 874313
Assignee: nobody → dhylands
Blocks: 893282
Blocks: 893284
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.
Attachment #775032 - Flags: review?(Jan.Varga)
Blocks: 881749
Any update with fix?
Flags: needinfo?(dhylands)
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?
Jan - any ETA on the review? I can find someone else if you're too busy.
Flags: needinfo?(dhylands) → needinfo?(Jan.Varga)
I'll try to finish it tomorrow.
Flags: needinfo?(Jan.Varga)
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
Attachment #775032 - Flags: review?(Jan.Varga) → review+
No longer blocks: 893282
Depends on: 893282
Duplicate of this bug: 881749
https://hg.mozilla.org/mozilla-central/rev/046e78d8a522
https://hg.mozilla.org/mozilla-central/rev/b8bb4f63c226
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Needs branch-specific patches for uplift.
Flags: needinfo?(dhylands)
Whiteboard: [fixed-in-birch]
Carrying r+

This patch doesn't have the whitespace changes, just the functional ones.
Attachment #789014 - Flags: review+
Flags: needinfo?(dhylands)
You need to log in before you can comment on or make changes to this bug.