Closed Bug 971612 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S3 (14mar)

People

(Reporter: alan.yenlin.huang, Assigned: alan.yenlin.huang)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 1 obsolete file)

No description provided.
Summary: Implement "mount", "unmount" API and for device storage → Implement "mount", "unmount" API for device storage
Hi Dave,

Can you help me to review this implementation? Thanks.
Attachment #8381309 - Flags: review?(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.
Attachment #8381309 - Flags: review?(dhylands) → review+
Revise patch according to reviewer's comments. Try server shows positive result.
Attachment #8381309 - Attachment is obsolete: true
Keywords: checkin-needed
Landing a new API without tests makes me sadface, BTW :(
Flags: in-testsuite?
(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.
https://hg.mozilla.org/mozilla-central/rev/64c6040bad03
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S3 (14mar)
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.
Attachment #8381311 - Flags: review?(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.
Attachment #8381311 - Flags: review?(dhylands) → feedback+
feature-b2g: --- → 2.0
feature-b2g: 2.0 → ---
You need to log in before you can comment on or make changes to this bug.