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

Status: VERIFIED FIXED

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

Description

[Alison Shiue]

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

Comment 1

[Alison Shiue]

[Blocking Requested - why for this release]:

Comment 2

[[e-mail], [e-mail]]

Per offline discussion with QA and UX, we will fix it to meet expected result.

Comment 3

[[e-mail], [e-mail]]

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.

Comment 4

[Alphan Chen [:alchen]]

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.

Comment 5

[Alphan Chen [:alchen]]

Attachment: (0911) Mark the volume as if we've started unmounting.

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.

Comment 6

[Dave Hylands [: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)

Comment 7

[Alphan Chen [:alchen]]

Attachment: (0916) Mark the volume as if we've started unmounting.

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.

Comment 8

[Dave Hylands [: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?

Comment 9

[Alphan Chen [:alchen]]

Attachment: Mark the volume as if we've started unmounting. r=dhylands

Here is try server result.
It looks good.
https://tbpl.mozilla.org/?tree=Try&rev=ce78914b17de

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e99d8abbc530

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval on this when you get a chance. Thanks :)

Comment 13

[Alphan Chen [: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

Comment 14

[bhavana bajaj [:bajaj]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/373b644d6c0a

Comment 16

[Alison Shiue]

Verified on Helix
Gaia-Rev        8061ab487d42cbc49b329fd68b9ca90e0fe477e6
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-aurora/rev/e970bc96f8b5
Build-ID        20140925000204
Version         34.0a2

Comment 17

[Elie]

Attachment: 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

Comment 18

[Mike Lien[:mlien]]

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