1035748 - [Device Storage] Introduce New Web API for Notifying Storage Added/Removed

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Marco Chen [:mchen]]

Bug 1035053 [Device Storage] To Support Variant Cloud Storage
(https://bugzilla.mozilla.org/show_bug.cgi?id=1035053#c1)
Bug 1035051 [Device Storage] To Support Network Storage via NFS, SAMBA and Others
Bug 1035050 [Device Storage] To Support USB Flash Drive 
(https://bugzilla.mozilla.org/show_bug.cgi?id=1035050#c1)

All user stories above show that there could be a storage been added or removed into/from storages array. Which is returned by navigator.getDeviceStorages() in runtime. To create this bug for focusing on this issue.

Comment 1

[Marco Chen [:mchen]]

Hi Dave,

According to the reason on the bug description, a event handler would be proposed to notify user storage changed.

The question is that I don't see any example of putting event handler (ex: ondevicestoragechanged) into navigator directly. All event handler was added inside a sub interface.

May I know you suggestion?
ex: add event handler into navigator directly or introduce a navigator.getDeviceStorageManager() then put things into "interface DeviceStorageManager". 

Thanks.

Comment 2

[Dave Hylands [:dhylands]]

I'm not sure what the best way to do this is. It's going to change the DOM so you whould probably have a discussion on the webapi mailing list.

Comment 3

[Marco Chen [:mchen]]

The discussion [1] was hosted on dev-webapi and there was no any feedback coming in already.
So to start the WebIDL feedback here then.

[1] https://groups.google.com/forum/#!searchin/mozilla.dev.webapi/[DeviceStorage]$20To$20propose$20new$20event$20handler$20for$20adding$2Fremoving$20/mozilla.dev.webapi/ol7sL7nM8KI/2FVz521eF_EJ

Comment 4

[Marco Chen [:mchen]]

Attachment: Part 1 - WebIDL

The WIP for DeviceStorageManager webidl.

Comment 5

[Marco Chen [:mchen]]

Attachment: Part 1 - WebIDL

Comment 6

[Marco Chen [:mchen]]

Attachment: Part 1 - WebIDL

Comment 7

[Marco Chen [:mchen]]

--------- DeviceStorageManager Web API ---------------

User Story:
1. As an user, when USB stick is plugged into TV, UI will show the name getting from USB stick but not the name set by system in default.
(Oppositely the name will be always "external sd card" when whatever sd card is plugged into TV.)
2. As an user, I can extend USB ports by USB hub so USB sticks can be plugged more then default number of USB ports on TV.

Technical points:
1. According to user stories, default set of device storages returned by getDeviceStorages() will not contain ones stood for USB sticks. The reasons are
--a. System can't know what name will be set by any USB sticks.
--b. System can't know how many device storages for USB sticks can be plugged in.
2. Since app can't get USB device storages in the beginning, it need a way to be notified of there is a new USB device storage added then get it.
3. Also USB device storage can be pulled out dynamically, app will need a way to be notified then drop it.
4. Currently app will listen status change (ex: available/non-available) from "each device storages" but in case of adding/removing dynamically
it should be better to have a central point for listening these changes. The reasons are
--a. For new USB device storage coming up, there is no existing device storage can be used to listen status change.
--b. For USB device storage pulled out, app can register into only one event listener instead of many of them. 

Referring to tech points, we propose to have a new event target called DeviceStorageManager under Navigator. And there will be a EventHandler for app to listen storages added/removed.

Comment 8

[Dave Hylands [:dhylands]]

Some of the points in comment 7 are incorrect.

It is possible to support USB sticks today (vold supports them - therefore device storage supports them). Supporting a single USB stick is not difficult. However, if you want to support some arbitrary number of them, then thngs get interesting.

The volume name is arbitrary and would be pre-assigned, just like it isfor sdcards. Removing a USB stick is handled in the same fashion as removing an external sdcard.

Comment 9

[Marco Chen [:mchen]]

(In reply to Dave Hylands [:dhylands] from comment #8)
> Some of the points in comment 7 are incorrect.

Thanks for correcting the wrong description.

> The volume name is arbitrary and would be pre-assigned
As I know that vold needs to maintain a volume list which contained the name of each volumes. (ex: sdcard, ext_sdcard) Therefore the support from vold can't leverage the name from USB stick itself which will be plug-in in run time. May I confirm this with you?

> if you want to support some arbitrary number of them, then thngs get interesting.
There is another use case for supporting un-known numbers of device storage in run time.
The use case is network storage including local network storage and cloud storage.
Users should be able to mount network storages as they want.
Thus we need a way to add/remove/listen in the run time.
Does it make sense?

Thanks.

Comment 10

[Dave Hylands [:dhylands]]

(In reply to Marco Chen [:mchen] from comment #9)
> (In reply to Dave Hylands [:dhylands] from comment #8)
> > Some of the points in comment 7 are incorrect.
> 
> Thanks for correcting the wrong description.
> 
> > The volume name is arbitrary and would be pre-assigned
> As I know that vold needs to maintain a volume list which contained the name
> of each volumes. (ex: sdcard, ext_sdcard) Therefore the support from vold
> can't leverage the name from USB stick itself which will be plug-in in run
> time. May I confirm this with you?

Correct. vold doesn't use the label for USB sticks, just like it doesn't use the label of an sdcard.
sdcards have labels just like USB thumb drives.

> > if you want to support some arbitrary number of them, then thngs get interesting.
> There is another use case for supporting un-known numbers of device storage
> in run time.
> The use case is network storage including local network storage and cloud
> storage.
> Users should be able to mount network storages as they want.
> Thus we need a way to add/remove/listen in the run time.
> Does it make sense?

Absolutely. I'm not arguing that we don't want this API, I just wanted to state that you could implement thumb stick support without it.

Comment 11

[Marco Chen [:mchen]]

(In reply to Dave Hylands [:dhylands] from comment #10)
Hi Dave,

Got it and thanks for the confirmation.

Comment 12

[Marco Chen [:mchen]]

Attachment: Part 1 WebIDL-v1.patch

Hi Ehsan,

Please refer to comment 3 and 7 for the detail of this feature.
And this will block us to support arbitrary number of storages from USB Sticks/Network Storages.

Thanks.

Comment 13

[(no longer active)]

Comment on attachment 8524542 [details] [diff] [review]
Part 1 WebIDL-v1.patch

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

::: dom/webidl/DeviceStorage.webidl
@@ +85,5 @@
> +  /**
> +   * According to some storage types (ex: USB Stick) may have the same name,
> +   * we need a way to identify which storage is actually removed from device.
> +   * The new attribute - id is added into DeviceStorage which is generated by
> +   * system and can be used as unique identifier.

So is this attribute unique per session?  Or is it guaranteed to be unique by device?

::: dom/webidl/DeviceStorageManager.webidl
@@ +8,5 @@
> +  "removed"
> +};
> +
> +dictionary DeviceStorageChangedEventInit : EventInit {
> +  DeviceStorageChangedEventOperation operation;

Please specify the default value in the IDL.

@@ +26,5 @@
> +[Pref="device.storage.enabled",
> + Constructor(DOMString type, optional DeviceStorageChangedEventInit eventInitDict)]
> +interface DeviceStorageChangedEvent : Event {
> +  readonly attribute DeviceStorageChangedEventOperation operation;
> +  readonly attribute DeviceStorage?                     storage;

What are the situations where storage may be null?  It seems to me like it should always be non-null.  Is it just for user created events?

@@ +29,5 @@
> +  readonly attribute DeviceStorageChangedEventOperation operation;
> +  readonly attribute DeviceStorage?                     storage;
> +}
> +
> +interface DeviceStorageManager : EventTarget {

You should use the [Pref] annotation here as well.

@@ +33,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);

Please split these parts into a separate bug.  This bug seems to only cover adding the event handler, so let's try to do things piecemeal.  (Of course you want to land the other patch first as it will add DeviceStorageManager, etc.)

Comment 14

[(no longer active)]

Dave, do we have any plans on supporting this for SD card hotplugging support on smartphones?

Comment 15

[Dave Hylands [:dhylands]]

I'm not sure its needed for sdcards on a phone. I believe that it is desirable for USB thumb-drive support (say on a TV device which has multiple USB ports), or using a USB-OTG cable on a phone to plug in a USB thumb drive (or a hub with multiple USB thumb drives).

Comment 16

[Marco Chen [:mchen]]

Yes, I agree with what Dave mentioned.
For SD card, it don't need this change but for USB and network storages they will need this.

Comment 17

[Kershaw Chang [:kershaw]]

Attachment: WebIDL for DeviceStorageChangedEvent - v2

Hi Ehasn,

Please take a look at this webidl for DeviceStorageChangedEvent.
Also some feedback below.

(In reply to :Ehsan Akhgari [Away: 1/29-2/20] (not reading bugmail, needinfo? me!) from comment #13)
> Comment on attachment 8524542 [details] [diff] [review]
> Part 1 WebIDL-v1.patch
> 
> Review of attachment 8524542 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/DeviceStorage.webidl
> @@ +85,5 @@
> > +  /**
> > +   * According to some storage types (ex: USB Stick) may have the same name,
> > +   * we need a way to identify which storage is actually removed from device.
> > +   * The new attribute - id is added into DeviceStorage which is generated by
> > +   * system and can be used as unique identifier.
> 
> So is this attribute unique per session?  Or is it guaranteed to be unique
> by device?
> 
I think making this id to be unique per session should be enough.


> @@ +26,5 @@
> > +[Pref="device.storage.enabled",
> > + Constructor(DOMString type, optional DeviceStorageChangedEventInit eventInitDict)]
> > +interface DeviceStorageChangedEvent : Event {
> > +  readonly attribute DeviceStorageChangedEventOperation operation;
> > +  readonly attribute DeviceStorage?                     storage;
> 
> What are the situations where storage may be null?  It seems to me like it
> should always be non-null.  Is it just for user created events?
> 
Actually, I am not sure if the storage should be null when this storage is removed.

Another question about DeviceStorageChangedEventOperation, should we also export more detailed information about the storage, such as "mounted" and "unmounted"? Or just simplify to "added" and "removed"?

Thanks.

Comment 18

[(no longer active)]

Comment on attachment 8555706 [details] [diff] [review]
WebIDL for DeviceStorageChangedEvent - v2

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

::: dom/webidl/DeviceStorage.webidl
@@ +91,5 @@
> +   * we need a way to identify which storage is actually removed from device.
> +   * The new attribute - id is added into DeviceStorage which is generated by
> +   * system and can be used as unique identifier.
> +   */
> +  readonly attribute unsigned long long id;

We should not expose an ID that is session only, since it is not obvious that the following code is wrong:

var ds = getDeviceStorageFromSomewhere();
writeDeviceStorageIDToIndexedDB(ds.id);

// Later on, in a new session
var dsId = readDeviceStorageIDFromIndexedDB();
var ds2 = getDeviceStorageFromSomewhereElse();
if (ds2.id == dsId) {
  // This will *never* happen!
}

Can't we just compare the objects to see if they are identical?

::: dom/webidl/DeviceStorageChangedEvent.webidl
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +enum DeviceStorageChangedEventOperation {
> +  "added",
> +  "removed",

On the question of whether we should add "mounted" or "unmounted" here, is there a use case to detect when the device is physically connected but unmounted?  (I think the answer is yes, if we want to show UI saying "It is now safe to remove the USB disk" and have the message disappear when you actually remove it.)  I can't think of a use case for "mounted" though.

@@ +27,5 @@
> +[Pref="device.storage.enabled",
> + Constructor(DOMString type, optional DeviceStorageChangedEventInit eventInitDict)]
> +interface DeviceStorageChangedEvent : Event {
> +  readonly attribute DeviceStorageChangedEventOperation operation;
> +  readonly attribute DeviceStorage                      storage;

This should not be null once the storage is removed, because otherwise you would not know which storage was removed.  You should however make sure that the promises returned from DeviceStorage after it has been removed will be rejected appropriately.

Comment 19

[Kershaw Chang [:kershaw]]

(In reply to [Away: 1/29-2/20] from comment #18)
> Comment on attachment 8555706 [details] [diff] [review]
> WebIDL for DeviceStorageChangedEvent - v2
> 
> Review of attachment 8555706 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/DeviceStorage.webidl
> @@ +91,5 @@
> > +   * we need a way to identify which storage is actually removed from device.
> > +   * The new attribute - id is added into DeviceStorage which is generated by
> > +   * system and can be used as unique identifier.
> > +   */
> > +  readonly attribute unsigned long long id;
> 
> We should not expose an ID that is session only, since it is not obvious
> that the following code is wrong:
> 
> var ds = getDeviceStorageFromSomewhere();
> writeDeviceStorageIDToIndexedDB(ds.id);
> 
> // Later on, in a new session
> var dsId = readDeviceStorageIDFromIndexedDB();
> var ds2 = getDeviceStorageFromSomewhereElse();
> if (ds2.id == dsId) {
>   // This will *never* happen!
> }
> 
> Can't we just compare the objects to see if they are identical?
> 
This id is for the case that we have some storages have the same name. I think maybe we don't need an additional id for this. Can we automatically add a serial number after the storage name, ?

> ::: dom/webidl/DeviceStorageChangedEvent.webidl
> @@ +4,5 @@
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +enum DeviceStorageChangedEventOperation {
> > +  "added",
> > +  "removed",
> 
> On the question of whether we should add "mounted" or "unmounted" here, is
> there a use case to detect when the device is physically connected but
> unmounted?  (I think the answer is yes, if we want to show UI saying "It is
> now safe to remove the USB disk" and have the message disappear when you
> actually remove it.)  I can't think of a use case for "mounted" though.
> 
> @@ +27,5 @@
> > +[Pref="device.storage.enabled",
> > + Constructor(DOMString type, optional DeviceStorageChangedEventInit eventInitDict)]
> > +interface DeviceStorageChangedEvent : Event {
> > +  readonly attribute DeviceStorageChangedEventOperation operation;
> > +  readonly attribute DeviceStorage                      storage;
> 
> This should not be null once the storage is removed, because otherwise you
> would not know which storage was removed.  You should however make sure that
> the promises returned from DeviceStorage after it has been removed will be
> rejected appropriately.

Comment 20

[Kershaw Chang [:kershaw]]

Sorry for the unfinished comment.

> > ::: dom/webidl/DeviceStorage.webidl
> > @@ +91,5 @@
> > > +   * we need a way to identify which storage is actually removed from device.
> > > +   * The new attribute - id is added into DeviceStorage which is generated by
> > > +   * system and can be used as unique identifier.
> > > +   */
> > > +  readonly attribute unsigned long long id;
> > 
> > We should not expose an ID that is session only, since it is not obvious
> > that the following code is wrong:
> > 
> > var ds = getDeviceStorageFromSomewhere();
> > writeDeviceStorageIDToIndexedDB(ds.id);
> > 
> > // Later on, in a new session
> > var dsId = readDeviceStorageIDFromIndexedDB();
> > var ds2 = getDeviceStorageFromSomewhereElse();
> > if (ds2.id == dsId) {
> >   // This will *never* happen!
> > }
> > 
> > Can't we just compare the objects to see if they are identical?
> > 
This id is for the case that we have some storages have the same name. I
think maybe we don't need an additional id for this. Can we automatically add a serial number after the storage name, such as "USB Stick1", "USB Stick2"?

> > ::: dom/webidl/DeviceStorageChangedEvent.webidl
> > @@ +4,5 @@
> > > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > > +
> > > +enum DeviceStorageChangedEventOperation {
> > > +  "added",
> > > +  "removed",
> > 
> > On the question of whether we should add "mounted" or "unmounted" here, is
> > there a use case to detect when the device is physically connected but
> > unmounted?  (I think the answer is yes, if we want to show UI saying "It is
> > now safe to remove the USB disk" and have the message disappear when you
> > actually remove it.)  I can't think of a use case for "mounted" though.
> > 
Yeah, you are correct. I think we don't need to expose "mounted" or "unmounted" here.
If a user want to know more detailed information, one can listen to "storage-state-change" event.

Comment 21

[Andrew Overholt [:overholt]]

Kershaw, Ehsan likely won't be able to get back to you until February 23 so you should ask someone else for feedback if you need it before then.  Maybe dhylands?

Comment 22

[Dave Hylands [:dhylands]]

(In reply to Kershaw Chang [:kershaw] from comment #20)
> Sorry for the unfinished comment.
> 
> > > ::: dom/webidl/DeviceStorage.webidl
> > > @@ +91,5 @@
> > > > +   * we need a way to identify which storage is actually removed from device.
> > > > +   * The new attribute - id is added into DeviceStorage which is generated by
> > > > +   * system and can be used as unique identifier.
> > > > +   */
> > > > +  readonly attribute unsigned long long id;
> > > 
> > > We should not expose an ID that is session only, since it is not obvious
> > > that the following code is wrong:
> > > 
> > > var ds = getDeviceStorageFromSomewhere();
> > > writeDeviceStorageIDToIndexedDB(ds.id);
> > > 
> > > // Later on, in a new session
> > > var dsId = readDeviceStorageIDFromIndexedDB();
> > > var ds2 = getDeviceStorageFromSomewhereElse();
> > > if (ds2.id == dsId) {
> > >   // This will *never* happen!
> > > }
> > > 
> > > Can't we just compare the objects to see if they are identical?
> > > 
> This id is for the case that we have some storages have the same name. I
> think maybe we don't need an additional id for this. Can we automatically
> add a serial number after the storage name, such as "USB Stick1", "USB
> Stick2"?

That makes sense to me. We probably wouldn't add the number to the first device, but I think that forcing the names to be unique (by the code appending some unique number at the end) is perfectly fine.

Comment 23

[Kershaw Chang [:kershaw]]

(In reply to Andrew Overholt [:overholt] from comment #21)
> Kershaw, Ehsan likely won't be able to get back to you until February 23 so
> you should ask someone else for feedback if you need it before then.  Maybe
> dhylands?

Thanks for the information. I'll ask Dave for feedback.

Comment 24

[Kershaw Chang [:kershaw]]

Attachment: WebIDL for DeviceStorageChangedEvent - v3

Update the patch.
As the discussion with dhylands, remove the id attribute.

Comment 25

[Kershaw Chang [:kershaw]]

Hi Dave,

Could you check the webidl again to see if this is reasonable to you?

Thanks.

Comment 26

[Dave Hylands [:dhylands]]

Comment on attachment 8558350 [details] [diff] [review]
WebIDL for DeviceStorageChangedEvent - v3

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

::: dom/webidl/DeviceStorageChangedEvent.webidl
@@ +13,5 @@
> + * For the EventHandler - onstoragechanged, new event is introduced and called
> + * DeviceStorageChangedEvent.
> + * The attribute - operation indicates that there is a device storage been
> + * added or removed.
> + * 1. If device storage is added, then attribute - storage is the added one

nit: I would probably reword this slightly.

1. If device storage is added, then the 'storage' attribute will contain the added storage (sentence continues on next line)

@@ +15,5 @@
> + * The attribute - operation indicates that there is a device storage been
> + * added or removed.
> + * 1. If device storage is added, then attribute - storage is the added one
> + * and can be used directly without calling getDeviceStorage(s) again.
> + * 2. If device storage is removed then attribute - storage indicates that

similarly:

2. If device storage is removed then the 'storage' attribute indicates the storage that was removed. (replaces this line and the next one)

Comment 27

[Gina Yeh [:gyeh] [:ginayeh]]

Andrew, can you find somebody to review the new WebIDL? Dave has provided some feedbacks in comment 26. Thanks.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8558350 [details] [diff] [review]
WebIDL for DeviceStorageChangedEvent - v3

This needs tests exercising the event constructor.  I assume those tests would have caught the following problem:

+  readonly attribute DeviceStorage                      storage;

says that .storage on the event is never null.  But the default value of .storage in the init dictionary is null, so just doing |new DeviceStorageChangedEvent("").storage| with this patch should crash...

You need to decide whether null is an allowed value there, and either consistently allow it or consistently disallow it.

Comment 29

[Kershaw Chang [:kershaw]]

Attachment: WebIDL for DeviceStorageChangedEvent - v4

I've updated the webidl as Dave's feedback and modified it to allow the storage can be null in event.
I think the storage is null only in the case that operation is equal to "unknown". However, I have no idea when this combination will happen.

@bz, does this modification make sense to you?

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8559671 [details] [diff] [review]
WebIDL for DeviceStorageChangedEvent - v4

> I think the storage is null only in the case that operation is equal to
> "unknown".

Clearly not:

  var ev = new DeviceStorageChangedEvent("", { operation: "added" });

This is OK, though it needs that newline at the end of file, but please do not land without adding those tests I asked for.  Speaking of which, this patch doesn't hook this event up to anything; I was assuming you're adding this to the generated events list, but if you're not I would want to see the actual implementation.

Comment 31

[Kershaw Chang [:kershaw]]

Attachment: WebIDL for DeviceStorageAreaChangedEvent

bz, it seems that you are not doing review right now.
If you don't mind, can I find somebody to review this idl (perhaps Ehsan)?

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8585378 [details] [diff] [review]
WebIDL for DeviceStorageAreaChangedEvent

Any DOM peer can review IDL changes, in general.  Ehsan would be fine, yes.

Anyway, I just looked this over and it's fine.

Comment 33

[Kershaw Chang [:kershaw]]

Attachment: WebIDL for DeviceStorageAreaChangedEvent, r=bz

Carry reviewer's name.

Test run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7ae30803cea

Comment 34

[Ryan VanderMeulen [:RyanVM]]

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

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8123a477b7d7