As bug 1035748 comment #13 described, this bug is for creating new API to manage device storage.

Hi Ehsan, Please take a look at this patch. Thanks.

Comment on attachment 8555718 [details] [diff] [review] WebIDL for DeviceStorageManager - v1 Review of attachment 8555718 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/DeviceStorageManager.webidl @@ +6,5 @@ > +[Pref="device.storage.enabled"] > +interface DeviceStorageManager : EventTarget { > + // The same functions of getDeviceStorage(s) under navigator will be kept > + // and note to be deprecated. > + sequence<DeviceStorage> getDeviceStorages(DOMString type); This should return a Promise, as it shouldn't be a synchronous API, I think. @@ +7,5 @@ > +interface DeviceStorageManager : EventTarget { > + // The same functions of getDeviceStorage(s) under navigator will be kept > + // and note to be deprecated. > + sequence<DeviceStorage> getDeviceStorages(DOMString type); > + DeviceStorage? getDeviceStorage (DOMString type); Ditto. Also, we should probably reject the promise if type is invalid, and not resolve with a null DeviceStorage. Also, please document what type can be. ::: dom/webidl/Navigator.webidl @@ +252,5 @@ > sequence<DeviceStorage> getDeviceStorages(DOMString type); > }; > > +partial interface Navigator { > + [Throws, Pref="device.storage.enabled", AvailableIn="PrivilegedApps"] Why the [AvailableIn]? @@ +253,5 @@ > }; > > +partial interface Navigator { > + [Throws, Pref="device.storage.enabled", AvailableIn="PrivilegedApps"] > + readonly attribute DeviceStorageManager deviceStorageManager; Please move this to the partial interface right above.

Thanks for the feedback. (In reply to [Away: 1/29-2/20] from comment #2) > Comment on attachment 8555718 [details] [diff] [review] > WebIDL for DeviceStorageManager - v1 > > Review of attachment 8555718 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/webidl/DeviceStorageManager.webidl > @@ +6,5 @@ > > +[Pref="device.storage.enabled"] > > +interface DeviceStorageManager : EventTarget { > > + // The same functions of getDeviceStorage(s) under navigator will be kept > > + // and note to be deprecated. > > + sequence<DeviceStorage> getDeviceStorages(DOMString type); > > This should return a Promise, as it shouldn't be a synchronous API, I think. > > @@ +7,5 @@ > > +interface DeviceStorageManager : EventTarget { > > + // The same functions of getDeviceStorage(s) under navigator will be kept > > + // and note to be deprecated. > > + sequence<DeviceStorage> getDeviceStorages(DOMString type); > > + DeviceStorage? getDeviceStorage (DOMString type); > > Ditto. Also, we should probably reject the promise if type is invalid, and > not resolve with a null DeviceStorage. > > Also, please document what type can be. > I add two new types: nfs and usbstick. I think nfs should be easy for implementation, so nfs will be supported first. > ::: dom/webidl/Navigator.webidl > @@ +252,5 @@ > > sequence<DeviceStorage> getDeviceStorages(DOMString type); > > }; > > > > +partial interface Navigator { > > + [Throws, Pref="device.storage.enabled", AvailableIn="PrivilegedApps"] > > Why the [AvailableIn]? > Yeah, you are right. We don't need this as the origin API doesn't have this restriction. > @@ +253,5 @@ > > }; > > > > +partial interface Navigator { > > + [Throws, Pref="device.storage.enabled", AvailableIn="PrivilegedApps"] > > + readonly attribute DeviceStorageManager deviceStorageManager; > > Please move this to the partial interface right above.

Comment on attachment 8556270 [details] [diff] [review] WebIDL for DeviceStorageManager - v2 Review of attachment 8556270 [details] [diff] [review]: ----------------------------------------------------------------- Looks reasonable to me, but I'm not really familiar with the nuances of webidl.

Comment on attachment 8556270 [details] [diff] [review] WebIDL for DeviceStorageManager - v2 Review of attachment 8556270 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/DeviceStorageManager.webidl @@ +9,5 @@ > + // and note to be deprecated. > + // Possible storage types are: apps, music, nfs, pictures, sdcard, videos, > + // usbstick. Note that nfs and usbstick are not implemented yet. > + Promise<sequence<DeviceStorage>> getDeviceStorages(DOMString type); > + Promise<DeviceStorage> getDeviceStorage (DOMString type); Does this break any existing code which uses the current navigator.getDeviceStorage/getDeviceStorages functions?

(In reply to Dave Hylands [:dhylands] from comment #5) > Comment on attachment 8556270 [details] [diff] [review] > WebIDL for DeviceStorageManager - v2 > > Review of attachment 8556270 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/webidl/DeviceStorageManager.webidl > @@ +9,5 @@ > > + // and note to be deprecated. > > + // Possible storage types are: apps, music, nfs, pictures, sdcard, videos, > > + // usbstick. Note that nfs and usbstick are not implemented yet. > > + Promise<sequence<DeviceStorage>> getDeviceStorages(DOMString type); > > + Promise<DeviceStorage> getDeviceStorage (DOMString type); > > Does this break any existing code which uses the current > navigator.getDeviceStorage/getDeviceStorages functions? No, it will not break the existing code. New API (getDeviceStorage/getDeviceStorages) can be only accessed via navigator.deviceStorageManager.

Hi bz, Could you please also review this webidl? Thanks.

Comment on attachment 8556270 [details] [diff] [review] WebIDL for DeviceStorageManager - v2 I can't review this without seeing the implementation...

And also, I'm not quite sure what the big comment is trying to say, fwiw.

(In reply to Boris Zbarsky [:bz] from comment #8) > Comment on attachment 8556270 [details] [diff] [review] > WebIDL for DeviceStorageManager - v2 > > I can't review this without seeing the implementation... I just want to make sure the API is correct before starting implementation. Anyway, I will start it right now. In the meantime, please let me know if there is anything wrong in this webidl. Thanks. (In reply to Boris Zbarsky [:bz] from comment #9) > And also, I'm not quite sure what the big comment is trying to say, fwiw. This bug, which is split out from bug 1035748, aims to introduce a new interface to manage device storage. A user can get device storage by this interface and also listen to DeviceStorageChangedEvent.

So normally, to figure out whether something is wrong in the IDL I would look at the following things: 1) Does the IDL match the spec? But in this case there is no spec I can see. 2) Does the spec's descriptions of what the various methods/attributes do make sense in light of the IDL? But there's no spec. 3) Does the IDL match the implementation? But there is no implementation to check against. Apart from not being able to check any of those things, the IDL looks fine, I guess. But it needs either a spec link or clear documentation of what the methods do. For example, what does getDeviceStorage do if you ask for a type other than one of the ones in the list? What does getDeviceStorages do? What exactly is the difference between getDeviceStorages and getDeviceStorage other than the return type? What do they do if you pass in a type that's in the list but there are no storages of that type around? As far as the comment, my problem is with this bit: + // The same functions of getDeviceStorage(s) under navigator will be kept + // and note to be deprecated. What is that trying to say? Is the idea something like: // These functions are meant to replace the now-deprecated functions with the same names // that are defined on the Navigator interface. ? If so, that seems like a comment that belongs on Navigator (pointing to this new API) instead....

Upload the WIP patch that uses CreateFakeVolume for proof of concept. Hi bz, Could you please help to take a look at this wip patch? I'd like to make sure my concept is correct. I have a question when a new storage is added. In deviceStorageManager, a new storage has to be created and returned to app in DeviceStorageChangedEvent. But, I am not sure which storage type should be used for the added storage. Should the type be the current types we have (music, sdcard, videos, ...) or should be the new type (e.g. nfs or usb)? Do we want to allow to save music or video in nfs storage? Thanks.

(In reply to Kershaw Chang [:kershaw] from comment #12) > But, I am not sure which storage type should be > used for the added storage. Should the type be the current types we have > (music, sdcard, videos, ...) or should be the new type (e.g. nfs or usb)? Do > we want to allow to save music or video in nfs storage? I think we should think of it as adding a storage area, and that storage area should be capable of storing all storage types. Perhaps, it may make sense for a storage area to indicate the types of storage it supports. For example, NFS, or dropbox would support all storage types. Imgur or picasa-web might only support photos.

Comment on attachment 8564942 [details] [diff] [review] WIP patch I don't really know what the answer is to your question about storage types; that depends on what the goals of this API are and there's no info in this bug about that.... Past that, this diff is missing the .webidl files, which makes it hard to say much about it, assuming you wanted my opinion on the IDL, not the storage types bit.

(In reply to Dave Hylands [:dhylands] from comment #13) > (In reply to Kershaw Chang [:kershaw] from comment #12) > > But, I am not sure which storage type should be > > used for the added storage. Should the type be the current types we have > > (music, sdcard, videos, ...) or should be the new type (e.g. nfs or usb)? Do > > we want to allow to save music or video in nfs storage? > > I think we should think of it as adding a storage area, and that storage > area should be capable of storing all storage types. > I assume the storage area you mean should be something like volume, right? I think we should reconsider the webidl. We should notify the user when there is a new volume is mounted. Then, the user can create a storage based on the new volume. > Perhaps, it may make sense for a storage area to indicate the types of > storage it supports. For example, NFS, or dropbox would support all storage > types. Imgur or picasa-web might only support photos. It also makes sense to me. I will add this idea in webidl.

(In reply to Boris Zbarsky [:bz] from comment #14) > Comment on attachment 8564942 [details] [diff] [review] > WIP patch > > I don't really know what the answer is to your question about storage types; > that depends on what the goals of this API are and there's no info in this > bug about that.... > > Past that, this diff is missing the .webidl files, which makes it hard to > say much about it, assuming you wanted my opinion on the IDL, not the > storage types bit. Thanks for your feedback. As the discussion in comment #15, maybe I should rethink about the idl.

(In reply to Kershaw Chang [:kershaw] from comment #15) > (In reply to Dave Hylands [:dhylands] from comment #13) > > (In reply to Kershaw Chang [:kershaw] from comment #12) > > > But, I am not sure which storage type should be > > > used for the added storage. Should the type be the current types we have > > > (music, sdcard, videos, ...) or should be the new type (e.g. nfs or usb)? Do > > > we want to allow to save music or video in nfs storage? > > > > I think we should think of it as adding a storage area, and that storage > > area should be capable of storing all storage types. > > > I assume the storage area you mean should be something like volume, right? > I think we should reconsider the webidl. We should notify the user when > there is a new volume is mounted. Then, the user can create a storage based > on the new volume. I've been trying to use the words "storage area" to be a bit more generic than "volume". On gonk, a volume is a storage area, but there are also non-volume based storage areas (i.e. just a plain directory tree). So I've been trying to use the terminology storageName rather than volumeName, storage area rather than volume, etc, except when actually dealing with volumes (which is a lower-level internal thing). Any IDLs should refer to storage areas and storage names and not refer to volumes.

Hi Dave, Could you please take a look at this webidl patch? The basic idea is to notify the user when a new storage area is added by DeviceStorageAreaChangeEvent. A new interface DeviceStorageArea is also introduced. It includes two attributes: type and name. Type indicates the file type that this storage area supports. The name has the same meaning as deviceStorage.storageName. A user can then use the storage area as parameter to create a DeviceStorage for accessing it. Thanks.

Comment on attachment 8571194 [details] [diff] [review] WebIDL for DeviceStorageAreaManager Review of attachment 8571194 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/DeviceStorageArea.webidl @@ +10,5 @@ > + "all" > +}; > + > +[Pref="device.storage.enabled"] > +interface DeviceStorageArea { I think that DeviceStorageArea is the wrong thing to call this. I would call it DeviceStorageNameAndType (see my other comment below). @@ +12,5 @@ > + > +[Pref="device.storage.enabled"] > +interface DeviceStorageArea { > + // The type indicates the file type that this storage area stores. > + readonly attribute DeviceStorageAreaType type; I think that this is insufficient. A storage area may support multiple storage types, or was the intention that if a storage area supports multiple types that multiple added events would be generated? ::: dom/webidl/Navigator.webidl @@ +255,5 @@ > DeviceStorage? getDeviceStorage(DOMString type); > [Throws, Pref="device.storage.enabled"] > sequence<DeviceStorage> getDeviceStorages(DOMString type); > + [Throws, Pref="device.storage.enabled"] > + DeviceStorage? getDeviceStorageByArea(DOMString type, DeviceStorageArea aStorageArea); This feels like incorrect use of the terminology to me. When I say storageArea, I'm usually referring to one of the DeviceStorage objects. You're really doing a lookup passing in a storageName and storageType and returning a storage area (aka DeviceStorage). I would be inclined to rename DeviceStorageArea to be DeviceStorageNameAndType and call this getDeviceStorageByNameAndType

(In reply to Dave Hylands [:dhylands] from comment #19) > Comment on attachment 8571194 [details] [diff] [review] > WebIDL for DeviceStorageAreaManager > > Review of attachment 8571194 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/webidl/DeviceStorageArea.webidl > @@ +10,5 @@ > > + "all" > > +}; > > + > > +[Pref="device.storage.enabled"] > > +interface DeviceStorageArea { > > I think that DeviceStorageArea is the wrong thing to call this. I would call > it DeviceStorageNameAndType (see my other comment below). > > @@ +12,5 @@ > > + > > +[Pref="device.storage.enabled"] > > +interface DeviceStorageArea { > > + // The type indicates the file type that this storage area stores. > > + readonly attribute DeviceStorageAreaType type; > > I think that this is insufficient. A storage area may support multiple > storage types, or was the intention that if a storage area supports multiple > types that multiple added events would be generated? > I defined a storage type "all" for the case that a storage area supports multiple types. Is this sufficient? The intention is to only generate one event when a new storage area becomes available. > ::: dom/webidl/Navigator.webidl > @@ +255,5 @@ > > DeviceStorage? getDeviceStorage(DOMString type); > > [Throws, Pref="device.storage.enabled"] > > sequence<DeviceStorage> getDeviceStorages(DOMString type); > > + [Throws, Pref="device.storage.enabled"] > > + DeviceStorage? getDeviceStorageByArea(DOMString type, DeviceStorageArea aStorageArea); > > This feels like incorrect use of the terminology to me. > > When I say storageArea, I'm usually referring to one of the DeviceStorage > objects. > Sorry that I am still confused about the terminology. Below is my understanding about "storage area" and "DeviceStorage". Please correct me if I am wrong. I think there is some difference between "storage area" and "DeviceStorage". A storage area, says nfs, supports all storage types. But a deviceStorage creates by navigator.getDeviceStorage("pictures") should only support pictures.

So I guess the exact definition of "storage area" isn't well defined, probably because there isn't actually anything in the API for a storage area. I could be convinced to use your definition of storage area, which is more or less analagous to a directory, or the place which a storageName refers to (and I probably use it the way you're suggesting too - I suspect I'm guilty of flopping my exact definition around since it isn't well defined). Given your definition of storage area, a DeviceStorageArea is definitely the wrong thing to call this thing: > interface DeviceStorageArea { > // The type indicates the file type that this storage area stores. > readonly attribute DeviceStorageAreaType type; > // The name of this storage area. > readonly attribute DOMString name; > }; That's really a way of identifying a DeviceStorage object. name by itself would be a way of identifying a storage area, but isn't by itself a storage area. The name & type might be called DeviceStorageId since its a way of identifying a DeviceStorage object, but then I though that might be confusing down the road. If we have a storageArea which is the "sort of analagous to a directory" idea, it may also make sense to have a storageId (some type of numeric equivalent to a storageName), and then storageId and DeviceStorageId seemed just a little too similar to refer to different things, which is why I suggested that we use DeviceStorageNameAndType.

(In reply to Dave Hylands [:dhylands] from comment #21) > So I guess the exact definition of "storage area" isn't well defined, > probably because there isn't actually anything in the API for a storage area. > > I could be convinced to use your definition of storage area, which is more > or less analagous to a directory, or the place which a storageName refers to > (and I probably use it the way you're suggesting too - I suspect I'm guilty > of flopping my exact definition around since it isn't well defined). > > Given your definition of storage area, a DeviceStorageArea is definitely the > wrong thing to call this thing: > > > interface DeviceStorageArea { > > // The type indicates the file type that this storage area stores. > > readonly attribute DeviceStorageAreaType type; > > // The name of this storage area. > > readonly attribute DOMString name; > > }; > > That's really a way of identifying a DeviceStorage object. > The reason why there is a type attribute is mentioned in comment #13. I think it makes sense to have a type attribute to let the user know the file type that this storage area supports, although in most of the cases the type should be "all". > name by itself would be a way of identifying a storage area, but isn't by > itself a storage area. > I think the name here should be equal to storageName in DeviceStorage object. Looking into the source code, the storageName is assigned to volume name in gonk. I don't quite understand why this name cannot be the storage area itself. Could you explain more or provide a example? > The name & type might be called DeviceStorageId since its a way of > identifying a DeviceStorage object, but then I though that might be > confusing down the road. > > If we have a storageArea which is the "sort of analagous to a directory" > idea, it may also make sense to have a storageId (some type of numeric > equivalent to a storageName), and then storageId and DeviceStorageId seemed > just a little too similar to refer to different things, which is why I > suggested that we use DeviceStorageNameAndType. I would like to summarize again about the definition of "storage area" and "DeviceStorage". Definition about storage area on mdn: A storage area is, in essence, a file system repository even if it hides the reality of the underlaying file system. In my understanding, it is something like a directory. In addition, I think we should add type attribute to indicate the file type that the storage area supports since some cloud storage may have this restriction. Definition about DeviceStorage on mdn: The DeviceStorage interface is used to access files on a specific storage area available on the device. To access a storage area, a user must use the navigator.getDeviceStorage() method, which returns DeviceStorage objects. The type is only used when creating a DeviceStorage object. In the current DeviceStorage interface, there is no type attribute. After a DeviceStorage is created, we have no way to know what the type is. So, I still think that type and name should be the way to identify a storage area, not DeviceStorage. Do you agree? Thanks.

I think maybe it's too early to think about adding type to storage area. Maybe we should focus on the current requirement: how to send a notification when a storage area is added or removed. How about let's create another bug in the future when we really need to support a photo-only cloud storage?

Sure - I think that makes sense. network shares (NFS, SAMBA), thumb drives and cloud storage (like DropBox and Google Drive) would all be of the "supports all types" variety.

> I think the name here should be equal to storageName in DeviceStorage > object. Looking into the source code, the storageName is assigned to volume > name in gonk. > I don't quite understand why this name cannot be the storage area itself. > Could you explain more or provide a example? Well storageName might be 'sdcard' which is just a name. The actual storage area is represented by a volume, which has a mount point and other attributes. So storageName is just a label, or a way to refer to a storageArea. > > The name & type might be called DeviceStorageId since its a way of > > identifying a DeviceStorage object, but then I though that might be > > confusing down the road. > > > > If we have a storageArea which is the "sort of analagous to a directory" > > idea, it may also make sense to have a storageId (some type of numeric > > equivalent to a storageName), and then storageId and DeviceStorageId seemed > > just a little too similar to refer to different things, which is why I > > suggested that we use DeviceStorageNameAndType. > > I would like to summarize again about the definition of "storage area" and > "DeviceStorage". > Definition about storage area on mdn: > A storage area is, in essence, a file system repository even if it hides the > reality of the underlaying file system. > In my understanding, it is something like a directory. In addition, I think > we should add type attribute to indicate the file type that the storage area > supports since some cloud storage may have this restriction. Yeah - I think that having a storage area be something like a directory is good. > Definition about DeviceStorage on mdn: > The DeviceStorage interface is used to access files on a specific storage > area available on the device. To access a storage area, a user must use the > navigator.getDeviceStorage() method, which returns DeviceStorage objects. > The type is only used when creating a DeviceStorage object. In the current > DeviceStorage interface, there is no type attribute. After a DeviceStorage > is created, we have no way to know what the type is. > So, I still think that type and name should be the way to identify a storage > area, not DeviceStorage. Do you agree? All initialized DeviceStorage instances have a type. That type is passed into the Init method, and is required. When you call GetDeviceStorage you pass in a type, and you'll only ever get back a DeviceStorage instance of that type. There isn't currently a method to retrieve the type associated with a DeviceStorage instance, but we could easily add one. It would return a copy of the mStorageType member. You can get the type of a DeviceStorage instance indirectly by calling DeviceStorage::GetRoot, which eventually returns a DeviceStorageFile object. You can query the type of a DeviceStorageFile, and it will match the type of the DeviceStorage object it comes from.

Dave, could you please take a look at this approach? Since we assume that storage area supports all file types now, the event only contains the name of the storage area. Then, this name can be used to create a DeviceStorage by navigator.getDeviceStorageByNameAndType. Thanks.

Hi Dave, Could you please take a look at this patch? I would like to make sure I am on the right track. In this patch, I am using nsIVolumeService.createFakeVolume to simulator the case that a new storage area is added. To test the removal case, I also add a new method removeFakeVolume in nsIVolumeService. Thanks.

Update the patch.

Dave, may I know your feedback? Thanks.

Comment on attachment 8577928 [details] [diff] [review] Bug-1126694-WIP Review of attachment 8577928 [details] [diff] [review]: ----------------------------------------------------------------- Sorry to take so long on this. For the most part this looks good to me. Just a bit of cleanup/refactoring. ::: dom/devicestorage/DeviceStorageAreaListener.cpp @@ +56,5 @@ > +#ifdef MOZ_WIDGET_GONK > + if (!strcmp(aTopic, NS_VOLUME_STATE_CHANGED)) { > + nsCOMPtr<nsIVolume> vol = do_QueryInterface(aSubject); > + if (!vol) { > + return NS_ERROR_NOT_AVAILABLE; Shouldn't this just be: MOZ_ASSERT(vol) Or is there some valid reason for vol to be NULL? @@ +68,5 @@ > + vol->GetName(volName); > + > + switch (state) { > + //XXX can we map this state to stand for adding a new storage area? > + case nsIVolume::STATE_INIT: I would probably ignore INIT. That state should only exist for a very short time when a volume object is being created. It basically means "I don't know what state I'm in". @@ +79,5 @@ > + mStorageAreaStateMap[volName] = DeviceStorageAreaChangedEventOperation::Added; > + } > + break; > + } > + //XXX can we map this state to stand for removal of a storage area? I think that any state other than MOUNTED should be considered to be removed. If there is no sdcard, it will just go INIT -> NOMEDIA. And you can transition to NOMEDIA from any other state if the user removes the sdcard. @@ +81,5 @@ > + break; > + } > + //XXX can we map this state to stand for removal of a storage area? > + case nsIVolume::STATE_IDLE: { > + auto iter = mStorageAreaStateMap.find(volName); Wouldn't it make sense to put this logic in DispatchStorageAreaChangedEvent? i.e. have it check the previous state and only send out a new event if the state changed? ::: dom/devicestorage/nsDeviceStorage.cpp @@ +3575,5 @@ > + if (NS_SUCCEEDED(rv)) { > + NS_ADDREF(*aStore = storage.get()); > + } > + return; > + } I think that the above can be replaced with a call to GetStorageByName (well almost). I think we should create a variation of GetStorageByName called GetStorageByNameAndType and haveGetStorageByName call GetStorageByNameAndType(aStorageName, mStorageType) ::: dom/devicestorage/test/test_storageAreaListener.html @@ +29,5 @@ > + var Ci = SpecialPowers.Ci; > + > + var volumeService = SpecialPowers.Cc["@mozilla.org/telephony/volume-service;1"].getService(Ci.nsIVolumeService); > + > + var volName = "nfs"; nit: Lets call this dummy-volume or something since it has nothing to do with NFS @@ +37,5 @@ > + if (navigator.deviceStorageAreaListener) { > + ok (true, "got deviceStorageAreaListener") > + } > + > + navigator.deviceStorageAreaListener.addEventListener("storageareachanged", function (e) { nit: indentation for lines 41 thru 54 doesn't match the rest of the file - probably a tabs versus spaces thing. ::: dom/system/gonk/nsVolumeService.cpp @@ +467,5 @@ > + false /* isUnmounting */, > + false /* isRemovable */, > + false /* isHotSwappable */); > + vol->LogState(); > + UpdateVolume(vol.get()); This doesn't seem like it actually removes the volume? In fact, it looks like it would create the volume if it didn't already exist.

(In reply to Dave Hylands [:dhylands] from comment #30) > Comment on attachment 8577928 [details] [diff] [review] > Bug-1126694-WIP > > Review of attachment 8577928 [details] [diff] [review]: > ----------------------------------------------------------------- > > Sorry to take so long on this. > > For the most part this looks good to me. Just a bit of cleanup/refactoring. Thanks for the feedback. > ::: dom/devicestorage/DeviceStorageAreaListener.cpp > @@ +56,5 @@ > > +#ifdef MOZ_WIDGET_GONK > > + if (!strcmp(aTopic, NS_VOLUME_STATE_CHANGED)) { > > + nsCOMPtr<nsIVolume> vol = do_QueryInterface(aSubject); > > + if (!vol) { > > + return NS_ERROR_NOT_AVAILABLE; > > Shouldn't this just be: MOZ_ASSERT(vol) > > Or is there some valid reason for vol to be NULL? No, it should be always non-null. > @@ +68,5 @@ > > + vol->GetName(volName); > > + > > + switch (state) { > > + //XXX can we map this state to stand for adding a new storage area? > > + case nsIVolume::STATE_INIT: > > I would probably ignore INIT. That state should only exist for a very short > time when a volume object is being created. It basically means "I don't know > what state I'm in". STATE_INIT is only for test purpose. A fake volume's state seems to be always STATE_INIT. I think I can add a check to see if it's a fake volume before dispatching storage area changed event. > @@ +79,5 @@ > > + mStorageAreaStateMap[volName] = DeviceStorageAreaChangedEventOperation::Added; > > + } > > + break; > > + } > > + //XXX can we map this state to stand for removal of a storage area? > > I think that any state other than MOUNTED should be considered to be > removed. If there is no sdcard, it will just go INIT -> NOMEDIA. And you can > transition to NOMEDIA from any other state if the user removes the sdcard. > > @@ +81,5 @@ > > + break; > > + } > > + //XXX can we map this state to stand for removal of a storage area? > > + case nsIVolume::STATE_IDLE: { > > + auto iter = mStorageAreaStateMap.find(volName); > > Wouldn't it make sense to put this logic in DispatchStorageAreaChangedEvent? > > i.e. have it check the previous state and only send out a new event if the > state changed? Thanks. It makes more sense to do this. > ::: dom/system/gonk/nsVolumeService.cpp > @@ +467,5 @@ > > + false /* isUnmounting */, > > + false /* isRemovable */, > > + false /* isHotSwappable */); > > + vol->LogState(); > > + UpdateVolume(vol.get()); > > This doesn't seem like it actually removes the volume? > > In fact, it looks like it would create the volume if it didn't already exist. Yes, this part looks really weird since I just want to reuse the code in UpdateVolume. I'll find another way to do this.

Update the patch based on the previous comments. I am still uncertain how to implement removeFakeVolume. There are some repeated code in SetFakeVolumeState. Please note that I also removed UpdateVolume in SetFakeVolumeState, since it seems that calling UpdateVolume from SetFakeVolumeState is always returned at [1]. [1] https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/nsVolumeService.cpp#389

(In reply to Kershaw Chang [:kershaw] from comment #31) > (In reply to Dave Hylands [:dhylands] from comment #30) > > @@ +68,5 @@ > > > + vol->GetName(volName); > > > + > > > + switch (state) { > > > + //XXX can we map this state to stand for adding a new storage area? > > > + case nsIVolume::STATE_INIT: > > > > I would probably ignore INIT. That state should only exist for a very short > > time when a volume object is being created. It basically means "I don't know > > what state I'm in". > > STATE_INIT is only for test purpose. A fake volume's state seems to be > always STATE_INIT. I think I can add a check to see if it's a fake volume > before dispatching storage area changed event. I think that's a bug/oversight. Initially fake volumes were just created for some testing things. We should probably make the fake volumes have a state of STATE_MOUNTED.

Comment on attachment 8585404 [details] [diff] [review] Part2: Impl of DeviceStorageAreaListener Review of attachment 8585404 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/devicestorage/DeviceStorageAreaListener.cpp @@ +65,5 @@ > + nsString volName; > + vol->GetName(volName); > + > + switch (state) { > + case nsIVolume::STATE_INIT: { I think that fake volumes should have their state be set to STATE_MOUNTED, and then this case can be deleted. ::: dom/system/gonk/nsVolumeService.cpp @@ +438,5 @@ > if (!vol || !vol->IsFake()) { > return NS_ERROR_NOT_AVAILABLE; > } > vol->SetState(state); > vol->LogState(); Why did you remove the call to UpdateVolume? That means that observers won't see state transitions. I think that fake volumes should start out in STATE_INIT (just like real volumes) and then change to STATE_MOUNTED. @@ +460,5 @@ > + return NS_ERROR_NOT_AVAILABLE; > + } > + vol->SetState(nsIVolume::STATE_NOMEDIA); > + vol->LogState(); > + NotifyVolumeStateChange(vol.get()); Shouldn't this remove the volume from mVolumeArray? Setting the state to NOMEDIA, and calling UpdateVolume seems fine (I think I'd rather see that than creating this new Notify function). I think that after doing this, it should go and remove the named volume from mVolumeArray. Otherwise mVolumeArray will just grow and grow if newly named volumes are added.

(In reply to Dave Hylands [:dhylands] from comment #36) > Comment on attachment 8585404 [details] [diff] [review] > Part2: Impl of DeviceStorageAreaListener > > Review of attachment 8585404 [details] [diff] [review]: > ----------------------------------------------------------------- > ::: dom/system/gonk/nsVolumeService.cpp > @@ +438,5 @@ > > if (!vol || !vol->IsFake()) { > > return NS_ERROR_NOT_AVAILABLE; > > } > > vol->SetState(state); > > vol->LogState(); > > Why did you remove the call to UpdateVolume? That means that observers won't > see state transitions. SetFakeVolumeState modified the volume in mVolumeArray [1] and then pass it to UpdateVolume. The modified volume is checked to see if it is equal to the volume with the same name in mVolumeArray. They are essentially two same volumes, so it returns at [2]. Observers actually has no chance to see state transition. Perhaps I should create a new volume in SetFakeVolumeState instead of modifying the current one in mVolumeArray? [1] https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/nsVolumeService.cpp#446 [2] https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/nsVolumeService.cpp#389

Summary of changes: 1. Review comments are addressed. 2. Add a new IPC to notify child process that a volume has been removed.

(In reply to Kershaw Chang [:kershaw] from comment #37) > (In reply to Dave Hylands [:dhylands] from comment #36) > > Comment on attachment 8585404 [details] [diff] [review] > > Part2: Impl of DeviceStorageAreaListener > > > > Review of attachment 8585404 [details] [diff] [review]: > > ----------------------------------------------------------------- > > ::: dom/system/gonk/nsVolumeService.cpp > > @@ +438,5 @@ > > > if (!vol || !vol->IsFake()) { > > > return NS_ERROR_NOT_AVAILABLE; > > > } > > > vol->SetState(state); > > > vol->LogState(); > > > > Why did you remove the call to UpdateVolume? That means that observers won't > > see state transitions. > > SetFakeVolumeState modified the volume in mVolumeArray [1] and then pass it > to UpdateVolume. The modified volume is checked to see if it is equal to the > volume with the same name in mVolumeArray. They are essentially two same > volumes, so it returns at [2]. Observers actually has no chance to see state > transition. > Perhaps I should create a new volume in SetFakeVolumeState instead of > modifying the current one in mVolumeArray? Looking at all of the other places that use UpdateVolume, they create a new volume, and pass that to UpdateVolume. So, I think that SetFakeVolumeState should make a copy of the found volume, update the state on that, and then call UpdateVolume. Then we'll get consistent behaviour. It's probably worth adding a comment at the beginning of UpdateVolume.

Comment on attachment 8586004 [details] [diff] [review] Part2: Impl of DeviceStorageAreaListener - v2 Review of attachment 8586004 [details] [diff] [review]: ----------------------------------------------------------------- Great - I think we're very close. ::: dom/devicestorage/DeviceStorage.h @@ +337,5 @@ > GetStorageByName(const nsAString &aStorageName); > > + static already_AddRefed<nsDOMDeviceStorage> > + GetStorageByNameAndType(nsPIDOMWindow* aWin, > + const nsAString &aStorageName, nit: put space after &, not before. ::: dom/devicestorage/DeviceStorageAreaListener.cpp @@ +85,5 @@ > +DeviceStorageAreaListener::DispatchStorageAreaChangedEvent( > + const nsString& aStorageName, > + DeviceStorageAreaChangedEventOperation aOperation) > +{ > + auto iter = mStorageAreaStateMap.find(aStorageName); I can't remember the last time I saw the use of the word auto. I'd recommend removing that. What you've really done here is declared iter as an int. You should specify an explicit type for iter. The way that I would normally do this is to create a typedef inside DeviceStorageAreaListener, something like: typedef std::map<nsString, DeviceStorageAreaChangedEventOperation> StateMapType; And then I would declare iter using either iterator or const_iterator (whichever is appropriate): StateMapType::const_iterator iter = mStorageAreaStateMap.find(aStorageName); ::: dom/devicestorage/DeviceStorageAreaListener.h @@ +29,5 @@ > + > + virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override; > + > +private: > + std::map<nsString, DeviceStorageAreaChangedEventOperation> mStorageAreaStateMap; And once you introduce StateMapType, then you can declare mStorageAreaStateMap using StateMapType; ::: dom/devicestorage/nsDeviceStorage.cpp @@ +3634,5 @@ > + > +// static > +already_AddRefed<nsDOMDeviceStorage> > +nsDOMDeviceStorage::GetStorageByNameAndType(nsPIDOMWindow* aWin, > + const nsAString &aStorageName, nit: swap position of & and space ::: dom/system/gonk/nsVolumeService.cpp @@ +423,5 @@ > false /* isRemovable */, > false /* isHotSwappable */); > vol->LogState(); > UpdateVolume(vol.get()); > + SetFakeVolumeState(name, nsIVolume::STATE_MOUNTED); SetFakeVolumeState will call LogState and UpdateVolume, and once SetFakeVolumeState is changed to create a new volume, then its call to UpdateVolume will actually do something. So I think that means that we should remove the calls to LogState and UpdateVolume here. @@ +476,5 @@ > + nsRefPtr<nsVolume> vol = FindVolumeByName(aName); > + if (!vol) { > + return; > + } > + mVolumeArray.RemoveElement(vol); Please put an opening curly brace in front of the MonitorAutoLock and a close curly brace after the call to RemoveElement. That will cause the Mutex to be released (at the cloing curly brace) when when autoLock object goes out of scope. Then we won't be holding the mutex when we call NotifyObservers below.

Review comments are addressed. > ::: dom/system/gonk/nsVolumeService.cpp > @@ +423,5 @@ > > false /* isRemovable */, > > false /* isHotSwappable */); > > vol->LogState(); > > UpdateVolume(vol.get()); > > + SetFakeVolumeState(name, nsIVolume::STATE_MOUNTED); > > SetFakeVolumeState will call LogState and UpdateVolume, and once > SetFakeVolumeState is changed to create a new volume, then its call to > UpdateVolume will actually do something. Please note that I use CreateOrFindVolumeByName to replace FindVolumeByName in SetFakeVolumeState if we removed UpdateVolume in CreateFakeVolume. We need to call CreateOrFindVolumeByName to add a new volume in mVolumeArray.

For helping the difference of v3 and v2.

Comment on attachment 8588962 [details] [diff] [review] Part2: Impl of DeviceStorageAreaListener - v3 Review of attachment 8588962 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I think it would be good to do a try run before landing.

Carry reviewer's name.

Rebase and carry reviewer's name.

bz, could you please review these webidl changes? If you want to see the implementation, it's in part2 patch. Thanks.

Comment on attachment 8590058 [details] [diff] [review] Part1: WebIDL for DeviceStorageAreaListener This looks ok as far as it goes, but I have some comments on part 2: 1) It leaks every single instance of DeviceStorageAreaListener that's ever created (because the RemoveObserver call is never reached). Given that this object is CCed and probably entrains the world, this seems like a bad idea. 2) Where is DeviceStorageAreaChangedEvent defined?

(In reply to Not doing reviews right now from comment #47) > Comment on attachment 8590058 [details] [diff] [review] > Part1: WebIDL for DeviceStorageAreaListener > > This looks ok as far as it goes, but I have some comments on part 2: > > 1) It leaks every single instance of DeviceStorageAreaListener that's ever > created (because the RemoveObserver call is never reached). Given that this > object is CCed and probably entrains the world, this seems like a bad idea. Thanks for pointing this out. I will try to fix this at next version. > 2) Where is DeviceStorageAreaChangedEvent defined? It's defined in bug 1035748.

bz, could you please take a look at this patch? I added Init() and Shutdown() for DeviceStorageListener and the instance of DeviceStorageListener will be freed in Navigator::Invalidate(). Thanks.

Comment on attachment 8593273 [details] [diff] [review] Part2: Impl of DeviceStorageAreaListener - v4 I'm not sure this solves the problem (and I encourage you to add testcases that would exercise this stuff to check whether it does). Specifically Navigator::Invalidate() is called from the following places: 1) nsGlobalWindow::CleanUp 2) nsGlobalWindow::FreeInnerObjects 3) Navigator unlink 4) ~Navigator If we get the device storage listener and add a property on it that points back to the navigator object, then #3 and #4 can't be reached, because the observer service keeps the listener alive, which keeps its preserved wrapper (since we added a property) alive, which keeps the Navigator alive. So the only question is whether we can do this after CleanUp/FreeInnerObjects have been called, or prevent them from being called after we add the prop. I'm not terribly confident that this can't happen. I think the sanest thing here is to have two objects: An observer object we add to the observer service and the device storage listener object. The observer would hold a weak reference to the device storage listener. The device storage listener would, in its constructor, create and take ownership of the observer add it to the observer service; in its destructor it would null out the backpointer to itself and remove the observer. Then the memory ownership is clear and can't possibly leak.

Thanks for the previous comment, bz. I've updated the patch as your suggestion. Could you please take a look?

Could you please post an interdiff from v4 to v5?

Sorry for missing the interdiff.

Comment on attachment 8595294 [details] [diff] [review] Part2: Impl of DeviceStorageAreaListener - v5 >+ DeviceStorageAreaListener* mDeviceStorageAreaListener; Please document that this is purposefully a weak ref and that mDeviceStorageAreaListener is responsible for nulling it out when it goes away (except yours doesn't). >+ obs->AddObserver(this, NS_VOLUME_STATE_CHANGED, false); Passing around a zero-refcount XPCOM object is usually a bad idea. Please don't do this inside the constructor; do it in an Init() method or something. >+ MOZ_ASSERT(mDeviceStorageAreaListener); I don't think you can reliably assert that. Just null check and return early here if it's null. >+DeviceStorageAreaListener::~DeviceStorageAreaListener() >+{ >+ mVolumeStateObserver = nullptr; >+} I'd really like to understand how you expect the memory ownership model here to work. Can you please explain that to me?

Oh, you probably want to annotate that raw pointer as MOZ_NON_OWNING_REF too.

Really sorry for wasting your time. I didn't think about memory ownership carefully since I forgot that observer service also increases the refcount of observer object. (In reply to Not doing reviews right now from comment #55) > Comment on attachment 8595294 [details] [diff] [review] > Part2: Impl of DeviceStorageAreaListener - v5 > > >+ DeviceStorageAreaListener* mDeviceStorageAreaListener; > > Please document that this is purposefully a weak ref and that > mDeviceStorageAreaListener is responsible for nulling it out when it goes > away (except yours doesn't). Should I make DeviceStorageAreaListener to support nsSupportsWeakReference? Can I just use rawptr here? > >+ obs->AddObserver(this, NS_VOLUME_STATE_CHANGED, false); > > >+DeviceStorageAreaListener::~DeviceStorageAreaListener() > >+{ > >+ mVolumeStateObserver = nullptr; > >+} > > I'd really like to understand how you expect the memory ownership model here > to work. Can you please explain that to me? I think the memory ownership model should be like: +-------------------------+ A +-------------------+ B +------------------+ |DeviceStorageAreaListener| +------> |VolumeStateObserver| <------+nsIObserverService| | | C | Refcnt:2 | | | +-------------------------+ <- - - + +-------------------+ +------------------+ A,B: strong reference C: weak reference In DeviceStorageAreaListener's destructor, I should remove A by setting mVolumeStateObserver to null and remove B by calling RemoveObserver. Please correct me if my idea is wrong. Thanks.

No need to apologize. Just good to be thinking about the ownership now. ;) > Can I just use rawptr here? Just using the rawptr is fine, with the MOZ_NON_OWNING_REF annotation. > In DeviceStorageAreaListener's destructor, I should remove A by setting > mVolumeStateObserver to null A will go away automatically when the refptr destructor runs. The important thing there is to remove _C_, so there is no dangling reference to the DeviceStorageAreaListener. Removing B is good too, I agree.

bz, please take a look at this patch. Thanks.

Comment on attachment 8595974 [details] [diff] [review] Part2: Impl of DeviceStorageAreaListener - v6 >+ void Forget() { mDeviceStorageAreaListener = nullptr; } I think ForgetListener would be clearer. r=me

(In reply to Not doing reviews right now from comment #61) > Comment on attachment 8595974 [details] [diff] [review] > Part2: Impl of DeviceStorageAreaListener - v6 > > >+ void Forget() { mDeviceStorageAreaListener = nullptr; } > > I think ForgetListener would be clearer. > > r=me Thanks for reviewing. Btw, should I need to request a super review for the idl changes?

I think it's fine for these changes.

That is, fine to not worry about superreview.

Carry reviewer's name.

Rebase and carry reviewer's name.

Rebase.

hi, can we get a try run for this changes ?

(In reply to Carsten Book [:Tomcat] from comment #68) > hi, can we get a try run for this changes ? oops, sorry that I forgot to put the link. https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdac8ddc19dd I just found that there are some failures that need to fix.

Hi Dave, The try run shows a error that is caused by the changes made in comment #41. Using CreateOrFindVolumeByName to create a new volume does not assign the mount point to the new volume. I think the solution could be creating a new volume in CreateFakeVolume and add it into mVolumeArray manually. May I know your thoughts on this patch? Thanks.

Comment on attachment 8598486 [details] [diff] [review] Part2: Impl of DeviceStorageAreaListener - v7 Review of attachment 8598486 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/nsVolumeService.cpp @@ +451,5 @@ > + { > + MonitorAutoLock autoLock(mArrayMonitor); > + mVolumeArray.AppendElement(vol); > + } > + SetFakeVolumeState(name, nsIVolume::STATE_MOUNTED); So now I'm confused again. I think that we could have kept the original code (calling UpdateVolume) and pass in STATE_MOUNTED rather than STATE_INIT when creating the new volume. UpdateVolume will wind up calling CreateOrFindVolumeByName which will create a new volume and add it to the array. The newly created volume won't be Equal to the volume we just created above (since the volume we created above will have a mount point, but the one created by CreateOrFindVolumeByName won't have a mount point. That will then trigger the notification. Or maybe I'm just confused by the back and forth.

(In reply to Dave Hylands [:dhylands] from comment #72) > Comment on attachment 8598486 [details] [diff] [review] > Part2: Impl of DeviceStorageAreaListener - v7 > > Review of attachment 8598486 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/gonk/nsVolumeService.cpp > @@ +451,5 @@ > > + { > > + MonitorAutoLock autoLock(mArrayMonitor); > > + mVolumeArray.AppendElement(vol); > > + } > > + SetFakeVolumeState(name, nsIVolume::STATE_MOUNTED); > > So now I'm confused again. > > I think that we could have kept the original code (calling UpdateVolume) and > pass in STATE_MOUNTED rather than STATE_INIT when creating the new volume. > > UpdateVolume will wind up calling CreateOrFindVolumeByName which will create > a new volume and add it to the array. The newly created volume won't be > Equal to the volume we just created above (since the volume we created above > will have a mount point, but the one created by CreateOrFindVolumeByName > won't have a mount point. > > That will then trigger the notification. > > Or maybe I'm just confused by the back and forth. Yes, you are correct. I can keep the original code. It's simpler.

Dave, please take a look. I just keep the original code of CreateFakeVolume and replace STAT_INIT with STATE_MOUNTED.

Treeherder looks green now. https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0b8bc361a79

Comment on attachment 8599374 [details] [diff] [review] Part2: Impl of DeviceStorageAreaListener - v8 Review of attachment 8599374 [details] [diff] [review]: ----------------------------------------------------------------- Excellent. ::: dom/system/gonk/nsVolumeService.cpp @@ +469,5 @@ > if (!vol || !vol->IsFake()) { > return NS_ERROR_NOT_AVAILABLE; > } > + > + nsRefPtr<nsVolume> volume = new nsVolume(name); I think its worth adding a comment above the new nsVolumne. Worded something like this: // UpdateVolume expects the volume passed in to NOT be the // same pointer as what CreateOrFindVolumeByName would return, // which is why we allocate a temporary volume here. No need to re-review.

Rebase

Rebase and add Dave's review comment.

(In reply to Dave Hylands [:dhylands] from comment #76) > Comment on attachment 8599374 [details] [diff] [review] > Part2: Impl of DeviceStorageAreaListener - v8 > > Review of attachment 8599374 [details] [diff] [review]: > ----------------------------------------------------------------- > > Excellent. > > ::: dom/system/gonk/nsVolumeService.cpp > @@ +469,5 @@ > > if (!vol || !vol->IsFake()) { > > return NS_ERROR_NOT_AVAILABLE; > > } > > + > > + nsRefPtr<nsVolume> volume = new nsVolume(name); > > I think its worth adding a comment above the new nsVolumne. Worded something > like this: > > // UpdateVolume expects the volume passed in to NOT be the > // same pointer as what CreateOrFindVolumeByName would return, > // which is why we allocate a temporary volume here. > > No need to re-review. Thanks for reviewing, Dave.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf66afc999a2 https://hg.mozilla.org/integration/mozilla-inbound/rev/aaa448765742 https://hg.mozilla.org/integration/mozilla-inbound/rev/c5d30567f419

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf66afc999a2 https://hg.mozilla.org/integration/mozilla-inbound/rev/aaa448765742 https://hg.mozilla.org/integration/mozilla-inbound/rev/c5d30567f419

https://hg.mozilla.org/mozilla-central/rev/bf66afc999a2 https://hg.mozilla.org/mozilla-central/rev/aaa448765742 https://hg.mozilla.org/mozilla-central/rev/c5d30567f419