959591 - Provide some ability to map sdcard to local directory with desktopb2g

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Zac C (:zac)]

To improve all forms of manual and automated testing we think it might be useful to be able to map an sdcard to a local folder/directory.

It could be something like /sdcard-1, /sdcard-2 for multiple sdcard support. The folder would sit inside the b2g or profile directory, perhaps like the emulator does.

desktopb2g would only have access to these directories rather than the entire host's storage. Limiting it to a folder would make file management easier for manual QA work and also automated work where we can automate the push/pull of files onto the 'sdcard' and not put the contents of the host drive at risk.

An immediate positive effect of this would to be able to run tests that use storage on CI jobs.

Comment 1

[Dietrich Ayala (:dietrich)]

Doug, where do we do this mapping?

Comment 2

[[:fabrice] Fabrice Desré]

here: https://mxr.mozilla.org/mozilla-central/source/dom/devicestorage/nsDeviceStorage.cpp#759

which leads to: https://mxr.mozilla.org/mozilla-central/source/dom/devicestorage/nsDeviceStorage.cpp#636

Comment 3

[Dietrich Ayala (:dietrich)]

So if I'm reading that right, looks like there's already support for a "sdcard" subfolder in the profile directory?

Comment 4

[Dietrich Ayala (:dietrich)]

Er, "fake-sdcard".

Comment 5

[[:fabrice] Fabrice Desré]

Yep, but the fact that it's in the profile directory makes it mostly useless I think. $HOME/fake-sdcard would be better no?

Comment 6

[Dietrich Ayala (:dietrich)]

Or for more flexibility, a pref, or runtime env var?

Comment 7

[[:fabrice] Fabrice Desré]

Sure. I would go with a sane default that we can override with a pref.

Comment 8

[Zac C (:zac)]

Wow, I asked Dave Hunt last week "wasn't there an sdcard/ folder in the profile directory at one stage?" and he replied no, so I thought I was going mad.. nice to know I'm not (although jury may still be out on that)!

If this still works - I'll check it tomorrow maybe it was removed from desktop profiles for a reason - then it may be enough for automation to work with already.

Comment 9

[Zac C (:zac)]

I can't seem to get this to work. I was only adding a "fake-sdcard" directory to the root of a profile generated by "DESKTOP_SHIMS=1 make" and then loading b2g. The directory is not generated during the make process.
Then I added content and started b2g, used the apps that consume this content. They still scanned the entire drive for media.

I may still be doing this wrong.

Comment 10

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Zac C (:zac) from comment #8)
> Wow, I asked Dave Hunt last week "wasn't there an sdcard/ folder in the
> profile directory at one stage?" and he replied no, so I thought I was going
> mad.. nice to know I'm not (although jury may still be out on that)!

I also asked you to raise this bug for it, and I'm glad I did! I've never seen this directory on desktop builds, but am always happy to be proven wrong... :) It will be great to have this extra coverage on TBPL/Travis!

Comment 11

[Zac C (:zac)]

Because this bug seems to demonstrate that this functionality exists at least in some capacity, I've raised https://bugzilla.mozilla.org/show_bug.cgi?id=964719 to track the functionality of fake-sdcard, but I'll keep this bug to track any enhancements we need to fake-sdcard to get it working like a device sdcard.

Comment 12

[Francesco Lodolo [:flod]]

Definitely wrong dependency.

Comment 13

[Zac C (:zac)]

Sorry flod!

Comment 14

[Dave Hylands [:dhylands]]

Currently on desktop (actual directory names may vary - this is just off the top of my head)

pictures is maped to ~/Pictures
music is mapped to ~/Music
video is mapped to ~/Video
sdcard is mapped to profile-dir/fake-sdcard

On a real phone, all 4 directories map to the same directory tree. And the files are filtered by type.

So on a real phone, you can write a file foo.jpg using the sdcard type and then read it back using the pictures type.
On desktop you can only read stuff back from the same type that you wrote it to.

So another sort of related question is do we want an option which maps all 4 storage types onto the same directory tree (which is kind of what setting the device.storage.testing does - they all map to /tmp/device-storage-testing (IIRC))

Maybe we just need a preference to control the actual location used when device.storage.testing is enabled?

Although device.storage.testing also disabled permissions checks.

Comment 15

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Dave Hylands [:dhylands] from comment #14)
> pictures is maped to ~/Pictures
> music is mapped to ~/Music
> video is mapped to ~/Video
> sdcard is mapped to profile-dir/fake-sdcard

This certainly reflects what I see on Mac, where my pictures appear in the Gallery app.

> So another sort of related question is do we want an option which maps all 4
> storage types onto the same directory tree (which is kind of what setting
> the device.storage.testing does - they all map to
> /tmp/device-storage-testing (IIRC))
> 
> Maybe we just need a preference to control the actual location used when
> device.storage.testing is enabled?

This should work for automation. We can create a temporary directory and populate it with files as the tests require them.

> Although device.storage.testing also disabled permissions checks.

What impact would this have on automation?

Comment 16

[Zac C (:zac)]

(In reply to Dave Hunt (:davehunt) from comment #15)
> (In reply to Dave Hylands [:dhylands] from comment #14)
> > 
> > Maybe we just need a preference to control the actual location used when
> > device.storage.testing is enabled?
> 
> This should work for automation. We can create a temporary directory and
> populate it with files as the tests require them.
> 
Yes I was kind of hoping this is how fake-sdcard would work already, but I've not had any luck getting it to work like that

Basically I just threw some pictures into the fake-sdcard folder that DS Test app creates but the Gallery app won't pick them up. It will only scan in ~/Pictures, not ~/Pictures and profile-dir/fake-sdcard/

Comment 17

[Dave Hylands [:dhylands]]

(In reply to Zac C (:zac) from comment #16)
> (In reply to Dave Hunt (:davehunt) from comment #15)
> > (In reply to Dave Hylands [:dhylands] from comment #14)
> > > 
> > > Maybe we just need a preference to control the actual location used when
> > > device.storage.testing is enabled?
> > 
> > This should work for automation. We can create a temporary directory and
> > populate it with files as the tests require them.
> > 
> Yes I was kind of hoping this is how fake-sdcard would work already, but
> I've not had any luck getting it to work like that
> 
> Basically I just threw some pictures into the fake-sdcard folder that DS
> Test app creates but the Gallery app won't pick them up. It will only scan
> in ~/Pictures, not ~/Pictures and profile-dir/fake-sdcard/

That's right. On desktop:

navigator.getDeviceStorage('sdcard') maps to fake-sdcard
navigator.getDeviceStorage('pictures') maps to ~/Pictures (this is what gallery uses)

So this is why putting pictures in the fake-sdcard directory aren't seen by gallery.

I think that we should add an override-pref that forces all of sdcard, pictures, video, and music to use a particular directory tree, and that this should override the location regardless of whether device.storage.testing is set or not. That should be a pretty simple change.

Comment 18

[Zac C (:zac)]

Dhylands, that sounds quite logical to me.

Comment 19

[Dave Hylands [:dhylands]]

Attachment: Allow device storage media dirs to be overridden (exp useful for desktop)

Comment 20

[[:fabrice] Fabrice Desré]

Comment on attachment 8369689 [details] [diff] [review]
Allow device storage media dirs to be overridden (exp useful for desktop)

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

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +687,5 @@
> +  if (overrideRootDir) {
> +    NS_NewLocalFile(overrideRootDir, false,
> +                    getter_AddRefs(sDirs->overrideRootDir));
> +    sDirs->overrideRootDir->Create(nsIFile::DIRECTORY_TYPE, 0777);
> +    sDirs->overrideRootDir->Normalize();

I think we want some error checking here in case we can't create the directory.

@@ +692,5 @@
> +
> +    nsString path;
> +    sDirs->overrideRootDir->GetPath(path);
> +    printf("device.storage.overrideRootDir set to '%s'\n",
> +           NS_LossyConvertUTF16toASCII(path).get());

nit: LOG() or remove

Comment 21

[Dave Hylands [:dhylands]]

Attachment: Allow device storage media dirs to be overridden (exp useful for desktop) v2

Added error checking.

Comment 22

[[:fabrice] Fabrice Desré]

Comment on attachment 8371194 [details] [diff] [review]
Allow device storage media dirs to be overridden (exp useful for desktop) v2

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

r=me
That would be nice to add tests.

Comment 23

[Dave Hylands [:dhylands]]

(In reply to Fabrice Desré [:fabrice] from comment #22)
> r=me
> That would be nice to add tests.

Righto - I've been working on stuff that's untestable for so long I forgot.

This should definitely be testable, so I will add some tests.

Comment 24

[Dave Hylands [:dhylands]]

So now I'm not sure how to go about testing this.

The way things are currently setup the preference is tested exactly once during when device storage is intantiated.

So in order to test this, we need to change the preference, restart B2G, run test, and then set the preference back and restart B2G again (so that it won't impact tests that follow).

I guess I'll need to find out from ateam how to actually accomplish this.

As well, regular mochitests seem to run tests in parallel, which we can't do for this particular test (since its a global setting, changing it while other tests are running runs the risk of screwing them up.

Comment 25

[Dave Hylands [:dhylands]]

Attachment: Add test for overrideRootDir preference

Comment 26

[Dave Hylands [:dhylands]]

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

Comment 27

[[:fabrice] Fabrice Desré]

Comment on attachment 8379212 [details] [diff] [review]
Add test for overrideRootDir preference

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

r=me

::: dom/devicestorage/test/test_overrideDir.html
@@ +16,5 @@
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=717103">Mozilla Bug 717103</a>
> +<p id="display"></p>
> +<div id="content" style="display: none">
> +  

nit: whitespace

@@ +24,5 @@
> +
> +devicestorage_setup();
> +
> +var gFileName = "devicestorage/" + randomFilename(12) + "/hi.png";
> +var gData = "My name is Doug Turner.  My IRC nick is DougT.  I like Maple cookies."

lol. instant r+ !

Comment 28

[Dave Hylands [:dhylands]]

fabrice - is it ok to land this on master? Or should I wait until 3/17 or 3/27?

Comment 29

[[:fabrice] Fabrice Desré]

(In reply to Dave Hylands [:dhylands] from comment #28)
> fabrice - is it ok to land this on master? Or should I wait until 3/17 or
> 3/27?

it's fine to land.

Comment 30

[Dave Hylands [:dhylands]]

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

Comment 31

[Dave Hylands [:dhylands]]

Hopefully this will fix the mochitest 8 failure

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

Comment 32

[Dave Hylands [:dhylands]]

https://hg.mozilla.org/integration/b2g-inbound/rev/cdcc9accb822
https://hg.mozilla.org/integration/b2g-inbound/rev/edfea182712f

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cdcc9accb822
https://hg.mozilla.org/mozilla-central/rev/edfea182712f

Comment 34

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I've just tested this with the latest B2G desktop build and gaiatest, and it works really well. Many thanks!

Comment 35

[Dave Hylands [:dhylands]]

Added dev-doc-needed to document the device.storage.overrideRootDir preference.