Closed Bug 1060196 Opened 5 years ago Closed 5 years ago

[Devices][Storage][Helix] Should pop up warning dialog when ejecting SD card while user is playing music which stored at SD card

Categories

(Firefox OS Graveyard :: Gaia::Settings, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.1+, firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.1 verified, b2g-v2.2 verified)

VERIFIED FIXED
2.1 S5 (26sep)
blocking-b2g 2.1+
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: ashiue, Assigned: alchen)

References

Details

Attachments

(2 files, 2 obsolete files)

Gaia      1934a2297ffc0d90424cd9cd3294c4a8c74a7333
Gecko     https://hg.mozilla.org/mozilla-central/rev/18901d4f3edd
BuildID   20140825160203
Version   34.0a1

STR:
1. Insert SD card which exists some music
2. Go to Music app and play a song
3. Go to Setting -> Media storage, press eject button for SD card

Expect result:
Pop-up a dialog to inform user that he could not eject SD card when files are using now

Actual result:
Nothing happen when press eject button, and after kill music app, the SD card would be ejected automatically
[Blocking Requested - why for this release]:
blocking-b2g: --- → 2.1?
QA Whiteboard: [COM=Storage]
Assignee: nobody → iliu
Per offline discussion with QA and UX, we will fix it to meet expected result.
blocking-b2g: 2.1? → 2.1+
Summary: [Devices][Storage] Should forbidden ejecting SD card when user is playing music which stored at SD card → [Devices][Storage][Helix] Should forbidden ejecting SD card when user is playing music which stored at SD card
Looks like Flame is working fine in this use case. But we still need to fix it on Helix. And it will need bug 1060201 supporting to callback in onerror case while the storage cannot be unmounted immediately.
Per discussion, we will follow the same behavior as doing "sharing" on 2.1.

Behavior:
After pressing "eject" bottom, 
we will close the file if there is any opened file first,
then we can unmount the sdcard.
In this patch,
Mark the volume as if we've started unmounting.
This will cause app which watch device storage notifications to see the volume go into different state and prompt them to close any opened file that they might have.

This behavior is the same as what we do when starting sharing.
For 2.1, We have the agreement with PM/UX.
Assignee: iliu → alchen
Status: NEW → ASSIGNED
Attachment #8487757 - Flags: feedback?(dhylands)
Comment on attachment 8487757 [details] [diff] [review]
(0911) Mark the volume as if we've started unmounting.

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

Your patches should have 8 lines of context.

See the last line of this section: https://developer.mozilla.org/en/docs/Mercurial_FAQ#How_can_I_diff_and_patch_files.3F

There are a couple of other bugs in play that this patch will mess up (bug 1040017 and bug 1063877), so it would be good if this one could wait until those have landed before this one does.

::: dom/system/gonk/AutoMounter.cpp
@@ +909,4 @@
>              break;
>            }
>  
> +          // Mark the volume as if we've started sharing/formatting/unmount.

nit: unmount s/b unmmounting.

::: dom/system/gonk/Volume.cpp
@@ +236,5 @@
>         mIsSharing = true;
>         break;
>  
> +     case nsIVolume::STATE_UNMOUNTING:
> +       mIsUnmounting = true;

I think that this should set

mUnmountRequested = false;
mIsFormatting = false;
mIsSharing = false;

and STATE_NOMEDIA, STATE_MOUNTED, STATE_FORMATTING

should set mIsUnmounting = false

I also think that STATE_SHARED should be setting

mIsUnformatting = false;
mIsFormatting = false;
mIsSharing = false;

::: dom/system/gonk/nsVolume.cpp
@@ +144,4 @@
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP nsVolume::GetIsUnmounting(bool *aIsUnmounting)

nit: move star: bool* aIsUnmounting;

@@ +275,4 @@
>  {
>    if (mState == nsIVolume::STATE_MOUNTED) {
>      LOG("nsVolume: %s state %s @ '%s' gen %d locked %d fake %d "
> +        "media %d sharing %d formatting %d unmountting %d",

nit: unmounting only has one t

::: dom/system/gonk/nsVolumeService.cpp
@@ +455,4 @@
>    {
>      MOZ_ASSERT(NS_IsMainThread());
>      DBG("UpdateVolumeRunnable::Run '%s' state %s gen %d locked %d "
> +        "media %d sharing %d formatting %d Unmounting %d",

nit: Unmounting s/b unmounting (no capital U)

@@ +476,4 @@
>  nsVolumeService::UpdateVolumeIOThread(const Volume* aVolume)
>  {
>    DBG("UpdateVolumeIOThread: Volume '%s' state %s mount '%s' gen %d locked %d "
> +      "media %d sharing %d formatting %d Unmounting %d",

nit: Unmounting b/b unmounting (no capital U)
Attachment #8487757 - Flags: feedback?(dhylands) → feedback+
Hi Dave,
all nits in comment 6 are picked except the following part.

1. 
case nsIVolume::STATE_UNMOUNTING:
mUnmountRequested = false;            
===> If I set this as false, SD card will be mounted again after unmounting.
     So I remove this part.

2.
case nsIVolume::STATE_SHARED
mIsSharing = false;
===> If I set this as false, there is a weird symptom.
     We cannot see "Unplug USB cable to ..." when disable USB storage from enable state. So I keep it as true in this patch.
Attachment #8487757 - Attachment is obsolete: true
Attachment #8492070 - Flags: review?(dhylands)
Comment on attachment 8492070 [details] [diff] [review]
(0916) Mark the volume as if we've started unmounting.

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

Looks good. Just one minor comment.

::: dom/system/gonk/Volume.cpp
@@ +242,5 @@
> +       break;
> +
> +     case nsIVolume::STATE_UNMOUNTING:
> +       mIsFormatting = false;
> +       mIsSharing = false;

Shouldn't mIsUnmounting be set to true here as well?
Attachment #8492070 - Flags: review?(dhylands) → review+
Here is try server result.
It looks good.
https://tbpl.mozilla.org/?tree=Try&rev=ce78914b17de
Attachment #8492070 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e99d8abbc530
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S5 (26sep)
Please request Aurora approval on this when you get a chance. Thanks :)
Flags: needinfo?(alchen)
Comment on attachment 8492966 [details] [diff] [review]
Mark the volume as if we've started unmounting. r=dhylands

Approval Request Comment
[Feature/regressing bug #]: Let the behavior of unmounting the same as sharing/formatting.
[User impact if declined]: The eject will malfunction if there is opened file. 
[Describe test coverage new/current, TBPL]: https://tbpl.mozilla.org/?tree=Try&rev=ce78914b17de
[Risks and why]: (Low risk) The behavior is the same as sharing and formatting.
[String/UUID change made/needed]:none
Attachment #8492966 - Flags: approval-mozilla-aurora?
Flags: needinfo?(alchen)
Comment on attachment 8492966 [details] [diff] [review]
Mark the volume as if we've started unmounting. r=dhylands

Requesting QA to help verify this as soon as it lands on 2.1
Attachment #8492966 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
Summary: [Devices][Storage][Helix] Should forbidden ejecting SD card when user is playing music which stored at SD card → [Devices][Storage][Helix] Should pop up warning dialog when ejecting SD card while user is playing music which stored at SD card
Verified on Helix
Gaia-Rev        8061ab487d42cbc49b329fd68b9ca90e0fe477e6
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-aurora/rev/e970bc96f8b5
Build-ID        20140925000204
Version         34.0a2
Status: RESOLVED → VERIFIED
Attached video VIDEO0049.mp4
This issue has been successfully verified on Flame 2.1:
Gaia-Rev        1b231b87aad384842dfc79614b2a9ca68a4b4ff3
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/95fbd7635152
Build-ID        20141119001205
Version         34.0
Device-Name     flame
FW-Release      4.4.2
verified and fixed with v2.2
Gaia-Rev        e5d666d6f62480ced56c6d9352f5e12befb5a862
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/5c4d07e2199e
Build-ID        20141124160209
Version         36.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141124.192635
FW-Date         Mon Nov 24 19:26:46 EST 2014
Bootloader      L1TC10011880
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.