971612 - Implement "mount", "unmount" API for device storage

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Comment 1

[Alan Huang]

Attachment: Implement "mount", "unmount" API for device storage, v1

Hi Dave,

Can you help me to review this implementation? Thanks.

Comment 2

[Alan Huang]

Attachment: ds-test patch to test attachment 8381809

Comment 3

[Dave Hylands [:dhylands]]

Comment on attachment 8381309 [details] [diff] [review]
Implement "mount", "unmount" API for device storage, v1

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

Looks good. I'd like to see a few asserts added for stuff that can only wortk in the parent process.

r=me with some added asserts

::: dom/system/gonk/AutoMounter.cpp
@@ +650,5 @@
>  }
>  
>  static void
> +AutoMounterMountVolumeIOThread(const nsCString& aVolumeName)
> +{

The AutoMounter only runs in the parent process, let's assert that in all of the AutoMounterXxxIOThread functions.

::: dom/system/gonk/nsVolume.cpp
@@ +205,5 @@
>    AutoMounterFormatVolume(aVolume);
>  }
>  
> +NS_IMETHODIMP nsVolume::Mount()
> +{

Mount, Unmount, and Format will only work if we're in the parent process, so lets add an assert to that effect.

@@ +215,5 @@
> +}
> +
> +/* static */
> +void nsVolume::MountVolumeIOThread(const nsCString& aVolume)
> +{

These variants also require being in the main process, so lets assert that.

Comment 4

[Alan Huang]

Attachment: Implement "mount", "unmount" API for device storage, v2. r=dhylands

Revise patch according to reviewer's comments. Try server shows positive result.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/64c6040bad03

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Landing a new API without tests makes me sadface, BTW :(

Comment 7

[Dave Hylands [:dhylands]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #6)
> Landing a new API without tests makes me sadface, BTW :(

I don't believe that our test infrastructure has any way of testing this functionality.

Testing involves plugging and unplugging USB cable. The emulator has no capability to simulate this, and we currently don't have any hardware which can be integrated with automation.

Maybe once bug 979391 is done and we have some of that hardware, then we can add proper tests.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/64c6040bad03

Comment 9

[Alan Huang]

Comment on attachment 8381311 [details] [diff] [review]
ds-test patch to test attachment 8381809 [details]

Hi Dave,

I would like to merge this into ds-test, for APIs format, mount/unmount and storageStatus. If this looks good to you, then I will do the pull request on github. Thanks.

Comment 10

[Dave Hylands [:dhylands]]

Comment on attachment 8381311 [details] [diff] [review]
ds-test patch to test attachment 8381809 [details]

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

I'm not comfortable with having Format format ALL volumes. But otherwise, this looks good.

::: test_apps/ds-test/js/ds-test.js
@@ +630,5 @@
> +      case 'storageStatus':
> +        report(new StorageStatus());
> +        break;
> +      case 'format':
> +        report(new Format());

You probably don't want to do this directly, as it will format ALL volumes on a device which has them.

Perhaps you should add another drop-down which is populated with the list of volumes, and have it have an "all" choice.

Then Format, Mount, Unmount should reject 'all' but would only work on the specified volume. report could then support all, or a specific volume.