Closed Bug 886627 Opened 7 years ago Closed 5 years ago

Enable device storage on Android

Categories

(Firefox for Android :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 34
Tracking Status
relnote-firefox --- 34+
fennec 33+ ---

People

(Reporter: wesj, Assigned: jchen)

Details

(Keywords: dev-doc-needed)

Attachments

(6 files, 1 obsolete file)

We should enable device storage on Android (bug 717103 https://wiki.mozilla.org/WebAPI/DeviceStorageAPI ). It should be as simple as flipping a pref:

device.storage.enabled
tracking-fennec: --- → ?
tracking-fennec: ? → 33+
It seems to me like changing device.storage.enabled just adds navigator.getDeviceStorage - but the request never finishes, i.e. neither "onsuccess" nor "onerror" are ever triggered.
Jim can you figure out what's going on here.
Assignee: nobody → nchen
Do you have a testcase, Tuxor?

I enabled the device.storage.enabled pref and the device storage tests in dom/devicestorage/test. Most of the tests pass and the few failures seem straightforward to fix.
Flags: needinfo?(tuxor1337)
I'm connecting via the debug console from desktop Firefox's developer tools. If device.storage.enabled is "true", then "navigator.getDeviceStorage" is a function. (If it's "false", then "navigator" doesn't have the property "getDeviceStorage".) I'm running navigator.getDeviceStorage("sdcard").available() and "readyState" is never leaving the value "pending". Actually, I would expect that there's a prompt inside of Firefox for Android asking for permissions to access the device storage. That is not the case.

I'm testing Firefox for Android v30.0 running on a Motorola Photon Q (Android 4.1.2).

Note that in dom/devicestorage/test the options "device.storage.testing" and "device.storage.prompt.testing" are also set. But these options don't exist in "about:config".
Flags: needinfo?(tuxor1337)
Another remark: When connecting to the Firefox OS Simulator 1.3 via about:app-manager, everything works just as expected in the debug console. navigator.getDeviceStorage("sdcard").available() has the property "readyState" and it's switching to "done" immediately after the request.
Ok, I see now "device.storage.testing" actually makes us use different directories for device storage. And we don't actually have code to handle picture/videos/music/sdcard directories on Android (in non-testing mode). It'll take some work but it's not hard.
Status: NEW → ASSIGNED
(In reply to Jim Chen [:jchen :nchen] from comment #6)
> Ok, I see now "device.storage.testing" actually makes us use different
> directories for device storage. And we don't actually have code to handle
> picture/videos/music/sdcard directories on Android (in non-testing mode).
> It'll take some work but it's not hard.

Can we consider making the tests actually test what would be run in the real world while we're at it. Geez.
This fixes device storage tests (on my Android device at least)
Attachment #8459817 - Flags: review?(dhylands)
Comment on attachment 8459817 [details] [diff] [review]
Fix device storage tests on Android (v1)

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

Looks reasonable to me.

I'm actually not sure why e10s would fail, since it all works properly with e10s.
Attachment #8459817 - Flags: review?(dhylands) → review+
This patch adds a call in AndroidBridge that in turn calls Java, to retrieve the full path to a public directory (downloads, music, etc.).
Attachment #8460288 - Flags: review?(rbarker)
This patch uses the call added in the previous patch to load root directories for device storage on Android.
Attachment #8460297 - Flags: review?(dhylands)
Comment on attachment 8460288 [details] [diff] [review]
Add AndroidBridge directory API (v1)

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

::: mobile/android/base/GeckoAppShell.java
@@ +2734,5 @@
>  
> +    /**
> +     * Retrieve the absolute path of an external storage directory.
> +     *
> +     * @param app Return the per-app directory if true.

This comment refers to a parameter (app) that does not exits.

@@ +2764,5 @@
> +            systemType = Environment.DIRECTORY_MUSIC;
> +        } else {
> +            return null;
> +        }
> +        return Environment.getExternalStoragePublicDirectory(type).getAbsolutePath();

Should this be systemType instead of type? systemType is set but never used otherwise.
Comment on attachment 8460297 [details] [diff] [review]
Use AndroidBridge directory API for device storage (v1)

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

Android has an implementation of the dirService in js here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/DirectoryProvider.js

Can we not extend it for this?
(In reply to Randall Barker [:rbarker] from comment #12)
> Comment on attachment 8460288 [details] [diff] [review]
> Add AndroidBridge directory API (v1)
> 
> Review of attachment 8460288 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/GeckoAppShell.java
> @@ +2734,5 @@
> >  
> > +    /**
> > +     * Retrieve the absolute path of an external storage directory.
> > +     *
> > +     * @param app Return the per-app directory if true.
> 
> This comment refers to a parameter (app) that does not exits.

Right, this comment line should be taken out.

> @@ +2764,5 @@
> > +            systemType = Environment.DIRECTORY_MUSIC;
> > +        } else {
> > +            return null;
> > +        }
> > +        return Environment.getExternalStoragePublicDirectory(type).getAbsolutePath();
> 
> Should this be systemType instead of type? systemType is set but never used
> otherwise.

Good catch. Should be systemType.
(In reply to Wesley Johnston (:wesj) from comment #13)
> Comment on attachment 8460297 [details] [diff] [review]
> Use AndroidBridge directory API for device storage (v1)
> 
> Review of attachment 8460297 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Android has an implementation of the dirService in js here:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/
> DirectoryProvider.js
> 
> Can we not extend it for this?

I think we still need to add a Java call to actually get the path, so we end up doing the same thing in a roundabout way.
(In reply to Jim Chen [:jchen :nchen] from comment #15)
> I think we still need to add a Java call to actually get the path, so we end
> up doing the same thing in a roundabout way.

Yeah. I was hoping it would mean less if-defs in dom/devicestorage/nsDeviceStorage.cpp (and it would mean that this is available to everyone on Android instead of just c++ code in this one place), but it looks like every platform has decided to add their own constants for Video/Music/etc dirs rather than creating platform independent ones so you'd still need ifdefs in nsDeviceStorage.cpp. aka. this code is kinda awful....
Under FirefoxOS we assume that Video/Music/Pictures/sdcard are all sharing the same directory tree, which is essentially what Android does (or at least did).

I looked on my Nexus 5 and the Gallery app seems to be alot more selective now that it was a year or so ago.

B2G Desktop is currently the only place where having separate trees is supported. Personally, I find the separate trees approach a bit rigid (you can't store album art in the Music tree for example).
Really need this feature
Comment on attachment 8460297 [details] [diff] [review]
Use AndroidBridge directory API for device storage (v1)

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

I Have no objections to this, although I also can't really make any comments as the the correctness of the implementation either.

I think that cleaning up the #ifdefs would be great, either by putting something into each platforms DirectoryService.js file (or similar), but that can be done in another bug.
Attachment #8460297 - Flags: review?(dhylands) → review+
This patch adds a test to make sure we can access regular device storage types outside of testing mode.
Attachment #8461298 - Flags: review?(dhylands)
Enable device storage API (don't need APK permission bump).
Attachment #8461299 - Flags: review?(mark.finkle)
Comment on attachment 8461298 [details] [diff] [review]
Add test for non-testing-mode device storage directories (v1)

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

I'm pretty sure that this will fail on TBPL if you do a try run.

The gaia mochitest app (gaia/dev_apps/mochitest/manifest.webapp) currently only has these permissions:

    "device-storage:music":{ "access": "readwrite" },
    "device-storage:pictures":{ "access": "readwrite" },
    "device-storage:sdcard":{ "access": "readwrite" },

and in non-b2g: (testing/mochitest/manifest.webapp)

    "device-storage:pictures":{ "access": "readwrite" },
    "device-storage:sdcard":{ "access": "readwrite" },

I have no issues with the tests. We just need to decide how we want to handle this. I think that adding the permissions in the two places above is fine, since we have an explicit test for testing the variations of app permissions (test_app_permissions.html).

So I'll mark it as r+ with the caveat that I don't think you'll be able to land it as-is.
Attachment #8461298 - Flags: review?(dhylands) → review+
Attachment #8461299 - Flags: review?(mark.finkle) → review+
(In reply to Dave Hylands [:dhylands][on PTO Wed Jul 23] from comment #22)
> Comment on attachment 8461298 [details] [diff] [review]
> Add test for non-testing-mode device storage directories (v1)
> 
> So I'll mark it as r+ with the caveat that I don't think you'll be able to
> land it as-is.

I tested it on try already. The test only calls gDS and does not use the returned object. AFAICT, gDS always succeeds even without permission, but you can't do anything with the returned object (i.e. all calls will fail). This is intended behavior, right?
(In reply to Jim Chen [:jchen :nchen] from comment #23)
> (In reply to Dave Hylands [:dhylands][on PTO Wed Jul 23] from comment #22)
> > Comment on attachment 8461298 [details] [diff] [review]
> > Add test for non-testing-mode device storage directories (v1)
> > 
> > So I'll mark it as r+ with the caveat that I don't think you'll be able to
> > land it as-is.
> 
> I tested it on try already. The test only calls gDS and does not use the
> returned object. AFAICT, gDS always succeeds even without permission, but
> you can't do anything with the returned object (i.e. all calls will fail).
> This is intended behavior, right?

Right - that makes sense. You can't really test the permission until you know what operation is going to be performed. So you can ignore my caveat :)
Addressed comments. rbarker said r+ over IRC.
Attachment #8460288 - Attachment is obsolete: true
Attachment #8460288 - Flags: review?(rbarker)
Attachment #8461813 - Flags: review+
Release Note Request (optional, but appreciated)
[Why is this notable]: New web API available for developers
[Suggested wording]: Enable Device Storage API for privileged apps
[Links (documentation, blog post, etc)]: https://developer.mozilla.org/en-US/docs/Web/API/Device_Storage_API
relnote-firefox: --- → ?
Added in the release notes with "Device Storage API for privileged apps enabled"

By the way, I think the documentation:
https://developer.mozilla.org/en-US/docs/Web/API/Device_Storage_API
needs an update ("Browser compatibility" at least)
I'm using Firefox Beta for Android, and setting device.storage.enabled to true still unable to use this API in installed app.
How can I test it?
(In reply to Nazar Mokrynskyi from comment #31)
> I'm using Firefox Beta for Android, and setting device.storage.enabled to
> true still unable to use this API in installed app.
> How can I test it?

It is only available to privileged apps, i.e. ones installed from the marketplace. You can test it in the simulator on desktop, but I am not sure if we have a way to test these directly on Fennec without using the marketplace. Maybe Myk knows?
Flags: needinfo?(myk)
I'm trying to install it from Marketplace, my app: https://marketplace.firefox.com/app/cleverstyle-music/
I made it temporary available for installation on Android.

By the way, apk do not ask me for any permissions about SD card (not sure whether it should, but I think so).
(In reply to Nazar Mokrynskyi from comment #33)
> I'm trying to install it from Marketplace, my app:
> https://marketplace.firefox.com/app/cleverstyle-music/
> I made it temporary available for installation on Android.
> 
> By the way, apk do not ask me for any permissions about SD card (not sure
> whether it should, but I think so).

The way these APKs work on Android, they use the permissions granted by the browser. The app APK doesn't neeed anything additional. We hope to change that soon-ish.

Anyway, give it a try in Nightly (Firefox 34). The necessary changes are only available there.
Nightly is not available in Google Play, can you point me somewhere I can find apk or simple instruction how to build it?
(In reply to Nazar Mokrynskyi from comment #35)
> Nightly is not available in Google Play, can you point me somewhere I can
> find apk or simple instruction how to build it?

http://nightly.mozilla.org/
I've installed Fennec on Android, app gets running, but when I try to enumerate files on music storage - neither "onsuccess" nor "onerror" callback called, namely, it still doesn't work. It is difficult to get any details since App Manager is unable to connect to debug app because of https://bugzilla.mozilla.org/show_bug.cgi?id=929382
Attached image DeviceStorage prompt
(In reply to Nazar Mokrynskyi from comment #37)
> I've installed Fennec on Android, app gets running, but when I try to
> enumerate files on music storage - neither "onsuccess" nor "onerror"
> callback called, namely, it still doesn't work. It is difficult to get any
> details since App Manager is unable to connect to debug app because of
> https://bugzilla.mozilla.org/show_bug.cgi?id=929382

I installed your app from the Marketplace and ran it using Nightly. I was able to see this permission prompt and play audio files.

I noticed a bug where the permission is not being remembered. I filed it as bug 1059907.
I've got permission prompt too, allowed that, and nothing.
Neither of callbacks was called.
I have LG Optimus G e970 phone with CyanogenMod 11 on it, and no SD card currently.
So, I have one mp3 file in the root of internal filesystem (anyway it is available as /sdcard and some other symlinks) and waiting few minutes that file was not found.
I've modified applization.zip of application (I have root permissions) but didn't succeed to make it working.
When you call getDeviceStorage("music"), Fennec will access the Android Music directory. The location of the Music directory is specific to the device. It can be one of the following (some of these may be symlinks),

/sdcard/Music
/storage/emulated/0/Music
/storage/emulated/legacy/Music
/storage/sdcard0/Music
/mnt/sdcard/Music
On B2G, we make music, video, pictures and sdcard types all use the same tree.

We did this because even though android has a Music folder, it allows music to stored and used outside of the Music folder. Also, with music, the artwork (which is a picture) is typically stored in the same folder as the music. Having separate trees means you have to maintain parallel trees in music and pictures.

Having the same trees used also makes it easier for apps like email which want to save an attachment. They can save it using the sdcard type, and it will get picked up by the music app when using the music type.

Otherwise the app which does the saving has to know the type of the file (and sometimes the differences are subtle, like a .ogg file with just an audio track is considered music, but if it has a video track then its considered video).
I've created directory, put one mp3 file there, directory is available on paths (they are all symlinks to one place):

/sdcard/Music
/storage/emulated/legacy/Music
/storage/sdcard0/Music
/mnt/sdcard/Music

Still no luck, can't find single file.
How to know where Firefox tries to find music and force it to pick up what I want it to.
Finally, yes, it works if installing app from marketplace, and not, it doesn't when using "mozilla-apk-cli" (I've used this one).
(In reply to Nazar Mokrynskyi from comment #43)
> Finally, yes, it works if installing app from marketplace, and not, it
> doesn't when using "mozilla-apk-cli" (I've used this one).

I inspected the Marketplace page for the app to find its mini-manifest URL <https://marketplace.firefox.com/app/a40104e3-1af9-4602-b22c-fc478715398a/manifest.webapp>, then read it to extract the package URL <https://marketplace.firefox.com/downloads/file/260869/cleverstyle-music-0.49.6.zip>, which I downloaded, and from which I generated a test/debug APK via mozilla-apk-cli:

> mozilla-apk-cli --overrideManifest https://marketplace.firefox.com/app/a40104e3-1af9-4602-b22c-fc478715398a/manifest.webapp cleverstyle-music-0.49.6.zip cleverstyle-music-0.49.6.apk

I then pushed it to my Nexus 4 test device:

> adb install -r cleverstyle-music-0.49.6.apk 

And ran it, whereupon it asked me to permit it to access my music files (multiple times, per bug 1059907) and then found and displayed the one music file on my device.

My device also notified me that debugging of the app was enabled, so I forwarded the port it specified (37931 in my case, but this changes each time you run the app) from my computer to the device:

> adb forward tcp:37931 tcp:37931

And then connected Firefox's remote developer tools to the port on which it was listening via the Tools > Web Developer > Connect… menu item (connecting to host "localhost" on port "37931"), whereupon Firefox opened a developer toolbox connected to the app.


So both the API and the test/debug APK work as expected for me.  But there could of course be bugs I'm not experiencing for whatever reason, so please file new bug reports if they continue to cause you problems!  Then we can diagnose and resolve those problems individually in their own comment threads, which'll be clearer than trying to do so in the comments on this bug.
Flags: needinfo?(myk)
You need to log in before you can comment on or make changes to this bug.