1033952 - Implement "isRemovable" API for device storage

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

According to bug 1007053 comment 22, comment 23, use case required(https://bugzilla.mozilla.org/show_bug.cgi?id=921105#c22), we need "isRemovable" API to expose volume configuration. Then, Settings App can decide how to deal with it.

We expect the value of "isRemovable":

true: The storage is able to removable without shut down the phone, unplug battery. Such as a removable sdcard, and the SDCard is not under the battery.

false: The storage is not able to removable while the phone is still running. An sdcard under the battery as non-removable (i.e. can't be removed while the phone is powered on).

Comment 1

[howie [:howie]]

Hi Dave, will you have enough resource to work on this? Thank you.

Comment 2

[Dave Hylands [:dhylands]]

(In reply to howie [:howie] from comment #1)
> Hi Dave, will you have enough resource to work on this? Thank you.

I'll probably be able to implement the core feature, however we'll need to figure out which devices support removable storage and add something for the ones which do. So I can help with the core, but I'd like somebody else to create the volume.cfg file for each phone which we determine to support removable media. I'll set it up to be non-removable by default, and the volume.cfg file will indicated that the media is removable (so we only need to create/update the volume.cfg for phones which actually support removable media).

Obviously, any device where you need to remove the battery to get at the sdcard slot should be considered non-removable.

The flame needs to be considered non-removable. The phones with a slide-in/slide-out card slot (like the dolphin or the unagi) seem to support media detection.

The flame sdcard connector style is not of the slide-in/slide-out variety, and in my testing, the flame doesn't detect card removal until you try to write to the non-existant volume, and it never again detects the volume being added.

Comment 3

[Hema Koka [:hema]]

Is this required for 2.1? Dave is currently on MTP and cannot get to this until the 2.1 commitments are wrapped up.

Thanks
Hema

Comment 4

[howie [:howie]]

Hi Hema, I'd consider this is good to have but not mandatory in 2.1, let's get other 2.1 commitments done first. thanks.

Comment 5

[Dave Hylands [:dhylands]]

I think that when we implement this we should actually have 2 attributes.

1 - Is the card physically removable? On a device like the flame this would be true.

2 - Is the card hot-swappable. Normally when the card requires the phone to be off in order to remove the card, they make it so you have to remove the battery to physically access the card. In the case of the flame, this is not the case. The card is physically removable, but not hot-swappable.

Distinguishing these 2 cases would allow the UI to display intelligent errors, for example, when no sdcard is detected.

Comment 6

[Eden Chuang[:edenchuang]]

Hello Dave

Could you give some comment about following implementation plan? Thanks.

According to currently implementation, InitVolumeConfig() is called before initialize and start VolumeManager. It means the we will get the configuration before we create Volume and maintain volumes in VolumeManager. It is a reasonable flow. 

Therefore, to support this enhancement, I think we have following things to do about implementation

1. Create a new command for user to describe volume's configuration in /system/etc/volume.cfg
2. A data structure to record the volume's configuration and a container to manager these configurations
3. Integrate configurations into actual Volume data structure and support API to corresponding information

For 1. I want to support a command with following syntax to 
configure <Volume Name> <Property Name> <Value>
ex. configurate sdcard1 removable 1

For 2. A new class VolumeConfig is created to record configurations, currently it is very simple as following

class VolumeConfig MozFinal
{
public:
  NS_INLINE_DECL_REFCOUNTING(VolumeConfig)
  VolumeConfig(const nsCSubstring& aName);
 
  ...

private:
  nsCString mName;
  bool      mIsRemovable;
};

To maintain the VolumeConfigs, I would like support a static VolumeConfig pointer array in VolumeManager and a series method to manager it. followings are the methods' phototyping

static VolumeConfigArray VolumeManager::mVolumeConfigs;
typedef nsTArray<RefPtr<VolumeConfig>> VolumeConfigArray;
static VolumeConfigArray::size_type NumVolumeConfigs();
static TemporaryRef<VolumeConfig> GetVolumeConfig(VolumeConfigArray::index_type aIndex);
static TemporaryRef<VolumeConfig> FindVolumeConfigByName(const nsCSubstring& aName);
static TemporaryRef<VolumeCofnig> FindAddVolumeConfigByName(const nsCSubstring& aName);

For 3, I want to keep current flow, reading /system/etc/volume.cfg first and then initializing and starting VolumeManager.

Therefore, to integrate VolumeConfig into this flow, we need to 

3.1 Parsing /system/etc/volume.cfg to get VolumeConfig and saving it into VolumeManager in InitVolumeConfig()
3.2 Adding the VolumeConfig to the corresponding Volume while the Volume is constructed by VolumeManager::FindAddVolumeByName().
3.3 Providing corresponding API in nsIVolume.idl, Volume and nsVolume to propagate information to Gaia

Comment 7

[Dave Hylands [:dhylands]]

As part of bug 1085743 I discovered that our "fake" volumes are only maintained on the user space thread (so available through nsVolueXxx but not through VolumeXxx), and not maintained on the IOThread volumes.

In order to fix this, I need to move InitVolumeConfig from the main thread onto the IOThread (and since its doing file I/O it should be on the IOThread anyways).

So then the logical place to read the volume.cfg file is AFTER we get the volume list from vold. Which means that we can perform modifications on the existing volumes (i.e. marking them as removable/red-only, etc).

So as an initial stab at this, I think that's the approach I would take. I don't think that we need to cache that config in memory.

So I would defer this bug until bug 1085743 is fixed, then it will essentially be trivial to implement. I expect to resume working on bug 1085743 early next week (if not today).

Comment 8

[Eden Chuang[:edenchuang]]

Attachment: Bug1033952-implement_"IsRemovable"_and_"IsHotSwappable"_API_for_device_storage_v1

Implement "IsRemovable" and "IsHotSwappable" API for device storage.

User can configure volume as "removable" and "hotswappable" through editing /system/etc/volume.cfg file.
New command "configure" is introduced for volume configuration.

syntax: configure <Volume Name> <Property Name> <Property Value>
  ex: configure sdcard REMOVABLE 0
      configure sdcard1 HOTSWAPPABLE 1

Currently only support two properties, "REMOVABLE" and "HOTSWAPPABLE".

Gaia also can get these device information through APIs provided by DeviceStorage.webidl.

Comment 9

[Eden Chuang[:edenchuang]]

Attachment: Bug1033952-implement_"IsRemovable"_and_"IsHotSwappable"_API_for_device_storage_(webidl)_v1

Implement "Removable" and "HotSwappable" APIs in webidl

Comment 10

[Eden Chuang[:edenchuang]]

(In reply to Eden Chuang[:edenchuang] from comment #8)
> Created attachment 8525109 [details] [diff] [review]
> Bug1033952-
> implement_"IsRemovable"_and_"IsHotSwappable"_API_for_device_storage_v1
> 
> Implement "IsRemovable" and "IsHotSwappable" API for device storage.
> 
> User can configure volume as "removable" and "hotswappable" through editing
> /system/etc/volume.cfg file.
> New command "configure" is introduced for volume configuration.
> 
> syntax: configure <Volume Name> <Property Name> <Property Value>
>   ex: configure sdcard REMOVABLE 0
>       configure sdcard1 HOTSWAPPABLE 1
> 
> Currently only support two properties, "REMOVABLE" and "HOTSWAPPABLE".

We have some assumptions on these two properties.
Since when we say a volume is "non-removable", reasonably, it should not be "hot-swappable".
And when we say a volume is "hot-swappable", we say it also be "removable".
So, once REMOVABLE is set as 0, we set the property HOTSWAPPABLE as 0 automatically. 
And, once HOTSWAPPABLE is set as 1, we set the property REMOVABLE as 1 automatically.

Since we configure volumes according to the command sequence in /system/etc/volume.cfg, if two configuration conflicts with each other, the latter one will be applied.
For example, 
   configure sdcard REMOVABLE 0
   configure sdcard HOTSWAPPABLE 1

In this case, the sdcard will be configured as HOTSWAPPABLE
 
> Gaia also can get these device information through APIs provided by
> DeviceStorage.webidl.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8525110 [details] [diff] [review]
Bug1033952-implement_"IsRemovable"_and_"IsHotSwappable"_API_for_device_storage_(webidl)_v1

r=me

Comment 12

[Dave Hylands [:dhylands]]

Comment on attachment 8525109 [details] [diff] [review]
Bug1033952-implement_"IsRemovable"_and_"IsHotSwappable"_API_for_device_storage_v1

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

Sorry to take so long on this review. You caught me just before I went on PTO last week.

This looks great.

I think that we should do some processing which sets removable/hotswappable based on the current heuristic.

So before the call to InitConfig here:
http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/VolumeManager.cpp#236
we should look at the number of volumes that we got from vold.

If there was only one volume, then mark it as removable/hot-swappable.
If there is more than one volume, mark each volume which has a name not equal to "sdcard" as removable/hotswappable.

Then we get the current behaviour without having to create a volume.cfg file for each and every phone that we've ever released.

The flame will need a volume.cfg to mark the external sdcard as non-removable.
Similarly, the nexus 4/5 will need a volume.cfg to mark sdcard as non-removable.

Clearing the review flag since I'd like to see this again after the cleanups.

::: dom/devicestorage/DeviceStorage.h
@@ +324,5 @@
>    nsCOMPtr<nsIFile> mRootDirectory;
>    nsString mStorageName;
>    bool mIsShareable;
> +  bool mIsRemovable;
> +  bool mIsHotSwappable;

Do we need both of these? Do we have any place where having both would benefit?

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +3391,5 @@
> +        return rv;
> +      }
> +      mIsRemovable = isRemovable;
> +      bool isHotSwappable;
> +      rv = vol->GetIsRemovable(&isHotSwappable);

Shouldn't this be GetIsHotSwappable?

::: dom/ipc/PContent.ipdl
@@ +484,5 @@
>      // Note: Any changes to this structure should also be changed in
>      // VolumeInfo above.
>      FileSystemUpdate(nsString fsName, nsString mountPoint, int32_t fsState,
>                       int32_t mountGeneration, bool isMediaPresent,
> +                     bool isSharing, bool isFormatting, bool isFake, bool isUnmounting, bool isRemovable, bool isHotSwappable);

nit: I think that line is > 80 cols, so please wrap it.

::: dom/system/gonk/Volume.cpp
@@ +165,5 @@
> +    return;
> +  }
> +  mIsRemovable = aIsRemovable;
> +  if (!mIsRemovable) {
> +    mIsHotSwappable = mIsRemovable;

Let's use mIsHotSwappable = false;

@@ +180,5 @@
> +    return;
> +  }
> +  mIsHotSwappable = aIsHotSwappable;
> +  if (mIsHotSwappable) {
> +    mIsRemovable = mIsHotSwappable;

Let's use mIsRemovable = true; I think its clearer.

@@ +190,5 @@
> +
> +Volume::CONFIGOPTION
> +Volume::GetConfigByString(const char* aConfigName)
> +{
> +  if (!strcmp(aConfigName, "REMOVABLE")) {

lets use strcasecmp

@@ +193,5 @@
> +{
> +  if (!strcmp(aConfigName, "REMOVABLE")) {
> +    return Volume::REMOVABLE;
> +  }
> +  if (!strcmp(aConfigName, "HOTSWAPPABLE")) {

same here

@@ +200,5 @@
> +  return Volume::UNKNOWN;
> +}
> +
> +void
> +Volume::ConfigVolume(const char* aConfigName, const char* aConfigValue)

I would rename this and call it something like SetConfig rather than ConfigVolume.

@@ +204,5 @@
> +Volume::ConfigVolume(const char* aConfigName, const char* aConfigValue)
> +{
> +  CONFIGOPTION config = GetConfigByString(aConfigName);
> +  switch (config) {
> +    case Volume::REMOVABLE: {

I think I wouldn't have bothered to have GetConfigByString and just put the call to strcasecmp here. Then you can get rid of the enum, and the GetConfigByString function.

@@ +211,5 @@
> +      } else if (!strcmp(aConfigValue, "0")) {
> +        SetIsRemovable(false);
> +      } else {
> +        ERR("Volume %s: invalid config value '%s' for config '%s'", NameStr(), aConfigValue, aConfigName);
> +      }

I think that the value parsing should be factored as well (i.e. write a function that takes a string and outputs a bool and returns a success/fail

::: dom/system/gonk/VolumeManager.cpp
@@ +172,5 @@
>  
>    ScopedCloseFile fp;
>    int n = 0;
>    char line[255];
> +  char *command, *volNamePtr, *mountPointPtr, *configNamePtr, *configValuePtr, *save_ptr;

Let's move configNamePtr and configValuePtr inside the command below.

I'd also probably move mountPointPtr inside the "create" command if.

command and save_ptr should go just before the strtok_r that assigns them.

And while you're doing that, I see this:
>     if (line[0] == '#')
>       continue;

@@ +202,5 @@
>        nsCString volName(volNamePtr);
>  
>        RefPtr<Volume> vol = FindAddVolumeByName(volName);
>        vol->SetFakeVolume(mountPoint);
> +    } else if (!strcmp(command, "configure")) {

I'd move the declarations for configNamePtr and configValuePtr inside this if.

@@ +203,5 @@
>  
>        RefPtr<Volume> vol = FindAddVolumeByName(volName);
>        vol->SetFakeVolume(mountPoint);
> +    } else if (!strcmp(command, "configure")) {
> +      // bug 1033952, New command 'configure' for configuring volume

No need to mention the bug number here. I'd remove that comment line entirely.

@@ +204,5 @@
>        RefPtr<Volume> vol = FindAddVolumeByName(volName);
>        vol->SetFakeVolume(mountPoint);
> +    } else if (!strcmp(command, "configure")) {
> +      // bug 1033952, New command 'configure' for configuring volume
> +      // Syntax: configure <VolumeName> <Perference> <PerferenceValue>

nit: s/Perference/Preference/ (both places)

@@ +211,5 @@
> +        ERR("No vol_name in %s line %d",  filename, n);
> +        continue;
> +      }
> +      if (!(configNamePtr = strtok_r(nullptr, delim, &save_ptr))) {
> +        ERR("No configuration for volume '%s'. %s line %d", volNamePtr, filename, n);

This should probably say "configuration name specified for volume ..."

@@ +225,5 @@
> +        vol->ConfigVolume(configNamePtr, configValuePtr);
> +      } else {
> +        ERR("Invalid volume name '%s'.", volNamePtr);
> +      }
> +    } else {

I think it would be a good idea to rework this whole thing to use nsCWhitespaceTokenizer (I realize you're just following what I coded in the first place).

It's used in VolumeManager.cpp and Volume.cpp (if you're looking for examples).

Then we don't need to be using bare char *

::: dom/system/gonk/nsVolume.cpp
@@ +409,5 @@
> +nsVolume::SetIsRemovable(bool aIsRemovable)
> +{
> +  mIsRemovable = aIsRemovable;
> +  if (!mIsRemovable) {
> +    mIsHotSwappable = mIsRemovable;

let's use mIsHotSwappable = false;

@@ +418,5 @@
> +nsVolume::SetIsHotSwappable(bool aIsHotSwappable)
> +{
> +  mIsHotSwappable = aIsHotSwappable;
> +  if (mIsHotSwappable) {
> +    mIsRemovable = mIsHotSwappable;

I'd just use mIsRemovable = true; here. I think it's a bit clearer.

::: dom/system/gonk/nsVolume.h
@@ +59,5 @@
>        mIsMediaPresent(false),
>        mIsSharing(false),
>        mIsFormatting(false),
> +      mIsUnmounting(false),
> +      mIsRemovable(false)

This should initialize mIsHotSwappable as well.

::: dom/system/gonk/nsVolumeService.cpp
@@ +398,5 @@
> +  bool aIsRemovable;
> +  aVolume->GetIsRemovable(&aIsRemovable);
> +  bool aIsHotSwappable;
> +  aVolume->GetIsHotSwappable(&aIsHotSwappable);
> +  nsRefPtr<nsVolume> vol = CreateOrFindVolumeByName(volName, aIsFake, aIsRemovable, aIsHotSwappable);

It isn't clear to me why you added removable and hot swappable as parameters to CreateOrFindVolumeByName

I would have expected those attrbiutes to get set as part of the Set call below.

Comment 13

[Eden Chuang[:edenchuang]]

Attachment: bug1033952-implement_"IsRemovable"_and_"IsHotSwappble"_APIs_for_device_storage_v2

Modify the patch according to the comment 12.

I add a new function VolumeManager::DefaultConfig() to set volume's config as following

If there was only one volume, then mark it as removable/hot-swappable.
If there is more than one volume, mark each volume which has a name not equal to "sdcard" as removable/hotswappable. 

and it will be called before VolumeManager::InitConfig() at
http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/VolumeManager.cpp#236

For the question mentioned in comment 12

::: dom/devicestorage/DeviceStorage.h
@@ +324,5 @@
>    nsCOMPtr<nsIFile> mRootDirectory;
>    nsString mStorageName;
>    bool mIsShareable;
> +  bool mIsRemovable;
> +  bool mIsHotSwappable;

Do we need both of these? Do we have any place where having both would benefit?

In fact, I have no idea how these attributes would be used in Gaia. I guess the meaning of "HotSwappable" is much more suitable for users, so I keep the hotswappable one. But I still use the name "Removable" to represent "HotSwappable" in nsDeviceStorage to keep the API name as original design.

Comment 14

[Dave Hylands [:dhylands]]

Comment on attachment 8530085 [details] [diff] [review]
bug1033952-implement_"IsRemovable"_and_"IsHotSwappble"_APIs_for_device_storage_v2

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

My apologies for taking so long. I had a family emergency just before Mozlandia and wound up being away for a week and a half.

This looks great. Just a few cleanups.

::: dom/system/gonk/Volume.cpp
@@ +194,5 @@
> +  if (aConfigValue.EqualsLiteral("1") ||
> +      aConfigValue.LowerCaseEqualsLiteral("true")) {
> +    aBoolValue = true;
> +    return true;
> +  } else if (aConfigValue.EqualsLiteral("0") ||

nit: No else after return

@@ +212,5 @@
> +      SetIsRemovable(value);
> +    } else {
> +      ERR("Volume %s: invalid value '%s' for configuration '%s'",
> +          NameStr(), aConfigValue.get(), aConfigName.get());
> +    }

nit: put a return here, and remove else a couple lines down.

@@ +221,5 @@
> +      SetIsHotSwappable(value);
> +    } else {
> +      ERR("Volume %s: invalid value '%s' for configuration '%s'",
> +          NameStr(), aConfigValue.get(), aConfigName.get());
> +    }

nit: put a return here and remove else a couple lines down

::: dom/system/gonk/VolumeManager.cpp
@@ +195,5 @@
> +
> +    nsCString command(tokenizer.nextToken());
> +    if (command.EqualsLiteral("create")) {
> +      if (!tokenizer.hasMoreTokens()) {
> +        ERR("no vol_name in %s line %d",  filename, n);

nit: 2 spaces after comma should be just 1

@@ +207,5 @@
> +      }
> +      nsCString mountPoint(tokenizer.nextToken());
> +      RefPtr<Volume> vol = FindAddVolumeByName(volName);
> +      vol->SetFakeVolume(mountPoint);
> +    } else if (command.EqualsLiteral("configure")) {

nit: put a continue in, and drop else

@@ +231,5 @@
> +        vol->SetConfig(configName, configValue);
> +      } else {
> +        ERR("Invalid volume name '%s'.", volName.get());
> +      }
> +    } else {

nit: put a continue in and drop else

@@ +241,5 @@
> +void
> +VolumeManager::DefaultConfig()
> +{
> +  VolumeManager::VolumeArray::size_type numVolumes = VolumeManager::NumVolumes();
> +  if (numVolumes == 1) {

Add a check for numVolumes == 0 and just return.

@@ +242,5 @@
> +VolumeManager::DefaultConfig()
> +{
> +  VolumeManager::VolumeArray::size_type numVolumes = VolumeManager::NumVolumes();
> +  if (numVolumes == 1) {
> +    RefPtr<Volume> vol = VolumeManager::GetVolume(0);

I think I'd put in a comment that this is to cover early shipping phones like the Buri, which had no internal storage, and only external sdcard.

Phones line the nexus-4 which only have an internal storage area will need to have a volume.cfg file with removable set to false.

@@ +245,5 @@
> +  if (numVolumes == 1) {
> +    RefPtr<Volume> vol = VolumeManager::GetVolume(0);
> +    vol->SetIsRemovable(true);
> +    vol->SetIsHotSwappable(true);
> +  } else {

nit: return early, unindent rest of the function

Comment 15

[Eden Chuang[:edenchuang]]

Attachment: bug1033952-implement_"IsRemovable"_and_"IsHotSwappble"_APIs_for_device_storage. r=dhylands

Comment 16

[Eden Chuang[:edenchuang]]

Comment on attachment 8539961 [details] [diff] [review]
bug1033952-implement_"IsRemovable"_and_"IsHotSwappble"_APIs_for_device_storage. r=dhylands

Approval Request Comment
[Feature/regressing bug #]: It is an enhancement for configuring volume removable/hotswappable status. 
[User impact if declined]: B2G has no idea if the sdcard is removable or not.
[Describe test coverage new/current, TBPL]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bdf306a099e
[Risks and why]: (Low risk) It is an enhancement and does not modify the original flow.
[String/UUID change made/needed]: none

Comment 17

[Eden Chuang[:edenchuang]]

Comment on attachment 8539961 [details] [diff] [review]
bug1033952-implement_"IsRemovable"_and_"IsHotSwappble"_APIs_for_device_storage. r=dhylands

Approval Request Comment
[Feature/regressing bug #]: It is an enhancement for configuring volume removable/hotswappable status. 
[User impact if declined]: B2G has no idea if the sdcard is removable or not.
[Describe test coverage new/current, TBPL]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bdf306a099e
[Risks and why]: (Low risk) It is an enhancement and does not modify the original flow.
[String/UUID change made/needed]: none

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c78cc72ea6d2
https://hg.mozilla.org/integration/b2g-inbound/rev/3263cf8aca77

Comment 19

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=1049377&repo=b2g-inbound

Comment 20

[Eden Chuang[:edenchuang]]

Attachment: bug1033952-implement_"IsRemovable"_and_"IsHotSwappble"_APIs_for_device_storage_webidl_v1. r=bzbarsky

Comment 21

[Eden Chuang[:edenchuang]]

Comment on attachment 8540691 [details] [diff] [review]
bug1033952-implement_"IsRemovable"_and_"IsHotSwappble"_APIs_for_device_storage_webidl_v1. r=bzbarsky

Approval Request Comment
[Feature/regressing bug #]: It is an enhancement for configuring volume removable/hotswappable status. 
[User impact if declined]: B2G has no idea if the sdcard is removable or not.
[Describe test coverage new/current, TBPL]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bdf306a099e
[Risks and why]: (Low risk) It is an enhancement and does not modify the original flow.
[String/UUID change made/needed]: none

Comment 22

[Eden Chuang[:edenchuang]]

(In reply to Carsten Book [:Tomcat] from comment #19)
> sorry had to back this out for bustage like
> https://treeherder.mozilla.org/logviewer.html#?job_id=1049377&repo=b2g-
> inbound

Sorry attached the wrong patches.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/41995e096743
https://hg.mozilla.org/integration/b2g-inbound/rev/248e0f88fa0a

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G debug mochitest asserts/crashes. Please verify that this is green on Try before requesting checkin on future revisions of these patches.
https://hg.mozilla.org/integration/b2g-inbound/rev/1fe76ae247cd

https://treeherder.mozilla.org/logviewer.html#?job_id=1053037&repo=b2g-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=1053038&repo=b2g-inbound

Comment 25

[Eden Chuang[:edenchuang]]

Hello Ryan

According to fail summary, I think these fails seem be not related to my patches.
I will run these unit-tests with my patches on Try again to make sure it.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

They sure went away post-backout and have mozilla::dom::VolumeInfo on the stack...

Comment 27

[Eden Chuang[:edenchuang]]

Need also update the VolumeInfo in

dxr.mozilla.org/mozilla-central/source/dom/system/gonk/nsVolumeService.cpp#253

Comment 28

[Eden Chuang[:edenchuang]]

Attachment: bug1033952-implement_"IsRemovable"_and_"IsHotSwappble"_APIs_for_device_storage. r=dhylands r=bzbarsky

Combine two patches into one patch and fix the unittests fail.

Comment 29

[Eden Chuang[:edenchuang]]

Comment on attachment 8541493 [details] [diff] [review]
bug1033952-implement_"IsRemovable"_and_"IsHotSwappble"_APIs_for_device_storage. r=dhylands r=bzbarsky

Approval Request Comment
[Feature/regressing bug #]: It is an enhancement for configuring volume removable/hotswappable status. 
[User impact if declined]: B2G has no idea if the sdcard is removable or not.
[Describe test coverage new/current, TBPL]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ced5b6743765
[Risks and why]: (Low risk) It is an enhancement and does not modify the original flow.
[String/UUID change made/needed]: none

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4995c6fc739e