Closed Bug 959591 Opened 6 years ago Closed 6 years ago

Provide some ability to map sdcard to local directory with desktopb2g

Categories

(Firefox OS Graveyard :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S2 (28feb)

People

(Reporter: zcampbell, Assigned: dhylands)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [fxos:media])

Attachments

(2 files, 1 obsolete file)

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.
Doug, where do we do this mapping?
Flags: needinfo?(doug.turner)
Flags: needinfo?(doug.turner)
So if I'm reading that right, looks like there's already support for a "sdcard" subfolder in the profile directory?
Er, "fake-sdcard".
Yep, but the fact that it's in the profile directory makes it mostly useless I think. $HOME/fake-sdcard would be better no?
Or for more flexibility, a pref, or runtime env var?
Sure. I would go with a sane default that we can override with a pref.
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.
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.
(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!
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.
Depends on: 946719
Definitely wrong dependency.
No longer depends on: 946719
Sorry flod!
Depends on: 964719
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.
(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?
(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/
(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.
Assignee: nobody → dhylands
Dhylands, that sounds quite logical to me.
Whiteboard: [fxos:media]
Target Milestone: --- → 1.4 S1 (14feb)
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
Attachment #8369689 - Flags: review?(fabrice) → feedback+
Added error checking.
Attachment #8369689 - Attachment is obsolete: true
Attachment #8371194 - Flags: review?(fabrice)
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.
Attachment #8371194 - Flags: review?(fabrice) → review+
(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.
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.
Attachment #8379212 - Flags: review?(fabrice)
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+ !
Attachment #8379212 - Flags: review?(fabrice) → review+
Target Milestone: 1.4 S1 (14feb) → 1.4 S2 (28feb)
fabrice - is it ok to land this on master? Or should I wait until 3/17 or 3/27?
Flags: needinfo?(fabrice)
(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.
Flags: needinfo?(fabrice)
Hopefully this will fix the mochitest 8 failure

https://tbpl.mozilla.org/?tree=Try&rev=77363cab6b65
I've just tested this with the latest B2G desktop build and gaiatest, and it works really well. Many thanks!
Added dev-doc-needed to document the device.storage.overrideRootDir preference.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.