Implement "isRemovable" API for device storage

RESOLVED FIXED in 2.2 S3 (9jan)

Status

Firefox OS
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: iliu@mozilla.com, ianliu.moz@gmail.com, Assigned: edenchuang)

Tracking

unspecified
2.2 S3 (9jan)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(firefox36 affected, firefox37 affected)

Details

Attachments

(1 attachment, 5 obsolete attachments)

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

3 years ago
Hi Dave, will you have enough resource to work on this? Thank you.
Flags: needinfo?(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.
Flags: needinfo?(dhylands)

Comment 3

3 years ago
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
Flags: needinfo?(hochang)

Comment 4

3 years ago
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.
Flags: needinfo?(hochang)
Blocks: 1055699
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.
(Assignee)

Comment 6

3 years ago
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
Flags: needinfo?(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).
Flags: needinfo?(dhylands)
(Assignee)

Updated

3 years ago
Assignee: nobody → echuang
(Assignee)

Comment 8

3 years ago
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".

Gaia also can get these device information through APIs provided by DeviceStorage.webidl.
Attachment #8525109 - Flags: review?(dhylands)
(Assignee)

Comment 9

3 years ago
Created attachment 8525110 [details] [diff] [review]
Bug1033952-implement_"IsRemovable"_and_"IsHotSwappable"_API_for_device_storage_(webidl)_v1

Implement "Removable" and "HotSwappable" APIs in webidl
Attachment #8525110 - Flags: review?(bzbarsky)
(Assignee)

Comment 10

3 years ago
(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 on attachment 8525110 [details] [diff] [review]
Bug1033952-implement_"IsRemovable"_and_"IsHotSwappable"_API_for_device_storage_(webidl)_v1

r=me
Attachment #8525110 - Flags: review?(bzbarsky) → review+
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.
Attachment #8525109 - Flags: review?(dhylands)
(Assignee)

Comment 13

3 years ago
Created attachment 8530085 [details] [diff] [review]
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.
Attachment #8525109 - Attachment is obsolete: true
Attachment #8530085 - Flags: review?(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
Attachment #8530085 - Flags: review?(dhylands) → review+
(Assignee)

Comment 15

3 years ago
Created attachment 8539961 [details] [diff] [review]
bug1033952-implement_"IsRemovable"_and_"IsHotSwappble"_APIs_for_device_storage. r=dhylands
Attachment #8530085 - Attachment is obsolete: true
(Assignee)

Comment 16

3 years ago
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
(Assignee)

Updated

3 years ago
Attachment #8539961 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8539961 - Flags: review+
(Assignee)

Updated

3 years ago
Target Milestone: --- → 2.2 S3 (9jan)
(Assignee)

Comment 17

3 years ago
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
Attachment #8539961 - Flags: approval-mozilla-aurora?
status-firefox36: --- → affected
status-firefox37: --- → affected
(Assignee)

Updated

3 years ago
Attachment #8539961 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/b2g-inbound/rev/c78cc72ea6d2
https://hg.mozilla.org/integration/b2g-inbound/rev/3263cf8aca77
Keywords: checkin-needed
sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=1049377&repo=b2g-inbound
Flags: needinfo?(echuang)
(Assignee)

Comment 20

3 years ago
Created attachment 8540691 [details] [diff] [review]
bug1033952-implement_"IsRemovable"_and_"IsHotSwappble"_APIs_for_device_storage_webidl_v1. r=bzbarsky
Attachment #8525110 - Attachment is obsolete: true
Flags: needinfo?(echuang)
(Assignee)

Comment 21

3 years ago
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
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 22

3 years ago
(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.
Flags: needinfo?(cbook)
Flags: needinfo?(cbook)
https://hg.mozilla.org/integration/b2g-inbound/rev/41995e096743
https://hg.mozilla.org/integration/b2g-inbound/rev/248e0f88fa0a
Keywords: checkin-needed
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
(Assignee)

Comment 25

3 years ago
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.
Flags: needinfo?(ryanvm)
They sure went away post-backout and have mozilla::dom::VolumeInfo on the stack...
Flags: needinfo?(ryanvm)
(Assignee)

Comment 27

3 years ago
Need also update the VolumeInfo in

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

Comment 28

3 years ago
Created attachment 8541493 [details] [diff] [review]
bug1033952-implement_"IsRemovable"_and_"IsHotSwappble"_APIs_for_device_storage. r=dhylands r=bzbarsky

Combine two patches into one patch and fix the unittests fail.
Attachment #8539961 - Attachment is obsolete: true
Attachment #8540691 - Attachment is obsolete: true
(Assignee)

Comment 29

3 years ago
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
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Attachment #8541493 - Attachment description: bug1033952_v4.patch → bug1033952-implement_"IsRemovable"_and_"IsHotSwappble"_APIs_for_device_storage. r=dhylands r=bzbarsky
https://hg.mozilla.org/integration/b2g-inbound/rev/4995c6fc739e
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4995c6fc739e
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.