893282 - Fix mediadb to scan all device storage areas

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Gallery, defect, P1)

Description

[Dave Hylands [:dhylands]]

Bug 885753 essentially removes support for composite storage areas, which implies that the MediaDB will need to be modified to scan all of the device storage areas, rather than relying on the composite device storage object to do it.

Nominating as leo? since this bug is tightly related to bug 885753, and that bug is leo+.

If this bug doesn't get leo+ then the gallery/video/music apps will only show files from the default device storage object.

Comment 1

[Dave Hylands [:dhylands]]

Attachment: Fix mediadb to scal all device storage areas.

Comment 2

[Leo]

Discussed with Mozilla both 885753 and 893282 should be applied. 885753 is already a Leo+ so marking this as Leo+.

Comment 3

[Dominic Kuo [:dkuo]]

Update: I have asked Mark to test this patch first because I haven't finish my review, and there are some bugs about the MediaDB that might be also resolved by it.

Comment 4

[David Flanagan [:djf]]

Comment on attachment 775024 [details] [diff] [review]
Fix mediadb to scal all device storage areas.

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

I haven't tested this patch because I haven't compiled the gecko patch. It looks to me like the code will work. I do have some changes to suggest below, however. I may not every actually be able to test this unless I can get new firmware on my Leo device, anyway.

I'm not certain that this enumeration patch is the only change needed.  If I understand the logic behind removing the composite storage stuff, I gather there are also user-facing changes in the expected behavior of apps when the default storage area is unavailable.

Currently, if the default storage area is the sdcard and the card is removed, MediaDB will still allow media apps to run.  (I'm not sure what happens if they try to save anything (like an edited photo). Possibly they break.)  Is that still the desired behavior, or should the apps only work when the default area is available?  Please check getStorageAvailability(), getState(), and volumeChangeHandler() inside of initDeviceStorage().  Does all of that code still look correct given these device storage changes?

::: shared/js/mediadb.js
@@ +1102,5 @@
>  
> +  // With the removal of composite storage, this function emulates
> +  // the composite storage enumeration (i.e. return files from
> +  // all of the storage areas).
> +  function EnumerateAll(storages, since) {

Since you're designing this to be used with the new operator, I'd prefer a noun as the class name, so we end up with code like

   var cursor = new EnumerateAllCursor(...);

But if it was me, I think I'd do it all with with nested functions inside of a single enumerateAll() function.  That would be a cleaner replacement for enumerate() because there wouldn't be the need to use 'new'.  And your code would be clearer because you could drop all the 'this' stuff.  And the code would be more robust because implementation details (like the i property) would be private.

I'd also change 'since' to 'options' because in theory there could be other options there, and the argument name since makes it very tempting to pass an integer there.

Also, I'd add another argument to mirror the path prefix argument of enumerate(), rather than hardcoding the empty string.

I also wonder whether this feature is generally useful enough that it should get its own file in shared/js/?  I already have shared/js/device_storage_utils.js, and I don't think it makes sense to add it to that file, though.

Comment 5

[Dominic Kuo [:dkuo]]

Comment on attachment 775024 [details] [diff] [review]
Fix mediadb to scal all device storage areas.

Since David started to review this and he is the author of mediadb, so I am re-assigning the review to him.

Comment 6

[David Flanagan [:djf]]

Comment on attachment 775024 [details] [diff] [review]
Fix mediadb to scal all device storage areas.

So Dominic wants me to do the formal review.

This is still untested, but r- for now pending:

1) an assessment of the the state management and notification code.  Does any of that need to change?

2) Either refactor EnumerateAll to be a function instead of a constructor, or rename to to be a noun instead of a verb.

Comment 7

[Dave Hylands [:dhylands]]

I forgot about the notification stuff (i.e how to react when volumes go away etc.).

As far as testing goes, I don't think you need the gecko side, as long as there are no calls to getDeviceStorage. getDeviceStorage will the function that changes, getDeviceStorages won't be changing.

So if mediadb has no calls to getDeviceStorage, then it should work the same whether the gecko side is patched or not.

I'll rework the patch to use a function (I'll probably have some questions for you) and test out the other stuff.

Comment 8

[Dave Hylands [:dhylands]]

Attachment: Revised as per guidance by djf

This is the updated version of the patch.

I filed bug 900159 to extract enumerateAll and the functions in shared/js/device_storage_utils.js

As far as notifications go (once bug 885753 lands), I think that mediadb is fine. It already determines what status to present by using getDeviceStorages (sdcard present or not)

The only thing mediadb.js uses getDeviceStorage for is to call addNamed, delete, and get. These functions have retained the bit of logic that allows fully qualified names to be specified, so I think we're good.

Comment 9

[David Flanagan [:djf]]

Comment on attachment 783928 [details] [diff] [review]
Revised as per guidance by djf

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

This code all looks good.  But insertFile() still has a call to media.storage.get() that can fail if some other app writes a media file to the non-default storage area.  Don't we need to simulate the composite get() function there by looking at the file prefix and selecting the correct device storage object?

Comment 10

[Dave Hylands [:dhylands]]

Comment on attachment 783928 [details] [diff] [review]
Revised as per guidance by djf

Bug 885753 (which removes most of the composite file support) retains some functionality in the get/addNamed/delete methods.

If you have a storage area for extsdcard say, and you call get('/sdcard/somefile') then it will redirect it to the correct storage area.

So putting back to r?

Comment 11

[David Flanagan [:djf]]

Comment on attachment 783928 [details] [diff] [review]
Revised as per guidance by djf

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

So you're saying that the device storage returned by getDeviceStorage() can read anything, but can only write to the default storage location? If so, then my concern about insertFile() was unfounded.  Sorry!

I'm giving r+ with two caveats:

1) If mediadb does media.storage = navigator.getDeviceStorage(), and then, while the app is still running, the user changes the default storage area in the settings database, does media.storage automatically pick that up and start writing to the new area? If not, then you should change the addFile() method to call getDeviceStorage() locally so that it always has a current object that writes to the correct area.

2) I have not tested this patch because my Leo device does not have up to date firmware and device storage just doesn't work on it at all.

Comment 12

[Dave Hylands [:dhylands]]

(In reply to David Flanagan [:djf] from comment #11)
> Comment on attachment 783928 [details] [diff] [review]
> Revised as per guidance by djf
> 
> Review of attachment 783928 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So you're saying that the device storage returned by getDeviceStorage() can
> read anything, but can only write to the default storage location? If so,
> then my concern about insertFile() was unfounded.  Sorry!
> 
> I'm giving r+ with two caveats:
> 
> 1) If mediadb does media.storage = navigator.getDeviceStorage(), and then,
> while the app is still running, the user changes the default storage area in
> the settings database, does media.storage automatically pick that up and
> start writing to the new area? If not, then you should change the addFile()
> method to call getDeviceStorage() locally so that it always has a current
> object that writes to the correct area.

Very good point. I hadn't considered this case, and putting the local call to getDeviceStorage seems like the right thing to do.

> 2) I have not tested this patch because my Leo device does not have up to
> date firmware and device storage just doesn't work on it at all.

On master, you can now create additional volumes by creating a: /system/etc/volume.cfg file which contains something like:

create storage /data/storage

You need to manually create /data/storage, and you can't share /data/storage via UMS, but gallery etc will see files put there.

Comment 13

[Dave Hylands [:dhylands]]

Attachment: Fixed addNamed to recall getDeviceStorage

The only change was in the function where addNamed was called.

Comment 14

[David Flanagan [:djf]]

Comment on attachment 785076 [details] [diff] [review]
Fixed addNamed to recall getDeviceStorage

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

::: shared/js/mediadb.js
@@ +931,5 @@
>        function save() {
> +        // Refetch the default storage area, since the user can change it
> +        // in the settings app.
> +        var storage = navigator.getDeviceStorage(media.mediaType);
> +        var request = storage.addNamed(file, filename);

I think you need to do this in the containing addFile function so that the delete call is also against the current default storage area, don't you?

And if you're making this change here, I'd do the same thing in insertFile() (and add a comment explaining that the default storage area will do the right thing for files prefixed with other storage areas).

And if you do that, then you can delete media.storage completely.

Comment 15

[David Flanagan [:djf]]

Comment on attachment 785076 [details] [diff] [review]
Fixed addNamed to recall getDeviceStorage

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

r+ if you make the changes described below

Comment 16

[Dave Hylands [:dhylands]]

https://github.com/mozilla-b2g/gaia/commit/9326784de7dc9e02d5d99dc1740febdb9a2b9327

Comment 17

[David Flanagan [:djf]]

I thought we still had to set status-b2g18 to affected in order to get bugs on jhford's radar for uplift. Maybe that is not needed anymore, but I'm doing it here just in case.

Comment 18

[David Flanagan [:djf]]

This patch causes a regression. Don't uplift this without also uplifting bug 901309.

Comment 19

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

Uplifted 9326784de7dc9e02d5d99dc1740febdb9a2b9327 to:
v1-train: 35d118484a5bc4b1c82992aa8db39aed97e7f9cd

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

v1.1.0hd: 35d118484a5bc4b1c82992aa8db39aed97e7f9cd