1204618 - [DeviceStorage] Add a new API to query the current low-disk-space status

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gabriele Svelto [:gsvelto]]

For 2.5 we want to enable the messages and dialer apps to stop storing any user data when the available storage space is running out and inform the user of what's happening. The only way we currently have to figure out if we're in a low-disk space scenario is via the 'change' event, however that prevents us from checking what's the status of the storage at app startup time. For example, if we're already in a low-space condition when the apps are launched the event handler won't be triggered until a certain amount of time has elapsed.

To work around this issue we'd like to have an interface to retrieve the low-disk-space status by querying it directly.

Comment 1

[Gabriele Svelto [:gsvelto]]

Attachment: bug-1204618-low-disk-space-query.patch

This is a very crude patch that adds a lowDiskSpace field to the DeviceStorage object. I'm not sure if this is the right way to go but at least it seems to work.

Comment 2

[Dave Hylands [:dhylands]]

Comment on attachment 8660903 [details] [diff] [review]
bug-1204618-low-disk-space-query.patch

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

Looks good to me, although I think you need to get a DOM Peer to review the webidl change.

Comment 3

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Add a field to the DeviceStorage object holding the low-disk-space status

I ended up using a slightly different approach that relies on the DiskSpaceWatcher event; this is reliable as we always trigger it at least once at startup and makes it so that the flag is always in-sync with the contents of the event delivered to the application. r? dhlyands for the device storage code and bzbarsky for the webidl changes.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8661200 [details] [diff] [review]
[PATCH] Add a field to the DeviceStorage object holding the low-disk-space status

"Indicates whether" or "True if", please.

r=me

Comment 5

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v2] Add a field to the DeviceStorage object holding the low-disk-space status. r=dhylands, r=bzbarsky

Thanks to both for the quick reviews! Updated patch with nits addressed, the try run is here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ad083b4a61a

Comment 6

[Gabriele Svelto [:gsvelto]]

After retriggering a lot of jobs with intermittent failures I've finally got a green try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ad083b4a61a

Time to land

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ffd40920af0c

Comment 8

[Phil Ringnalda (:philor)]

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

Comment 9

[Julien Wajsberg [:julienw]]

Comment on attachment 8662017 [details] [diff] [review]
[PATCH v2] Add a field to the DeviceStorage object holding the low-disk-space status. r=dhylands, r=bzbarsky

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

A last-minute comment, sorry that I look at this only after it landed...

::: dom/webidl/DeviceStorage.webidl
@@ +86,5 @@
>    // Indicates if the storage area denoted by storageName is removable
>    readonly attribute boolean isRemovable;
>  
> +  // True if the storage area is close to being full
> +  readonly attribute boolean lowDiskSpace;

Mmm I would have used a String actually; because in the future we'll have a 3-state value: "normal situation", "close to low-storage condition", "low-storage condition".

But if we use "" (or null), "near-low", "low" it could be backward compatible, assuming developers don't check for the actual boolean value.

Comment 10

[Boris Zbarsky [:bzbarsky]]

If we think we'll want a multi-state thing here in the future, we should just go ahead and use an IDL enum.  But we'd also want a different name for it, then, yes?

Comment 11

[Julien Wajsberg [:julienw]]

Ah yeah, right, enum is the right approach here.

About the name, I'm bad at naming, but I think "lowDiskSpace" still makes sense, since it would hold the status of being near or in low storage condition.

Comment 12

[Boris Zbarsky [:bzbarsky]]

I'd think "diskSpaceStatus" would make more sense there.

Comment 13

[Julien Wajsberg [:julienw]]

why not !

Comment 14

[Gabriele Svelto [:gsvelto]]

OK, well this mirrors the internal implementation for now (yeah, I know, not nice for an API) but we're free to change it when we also change the event since they are coupled.

Comment 15

[Chris Mills [:cmills]]

I've added the new property to this page:

https://developer.mozilla.org/en-US/docs/Web/API/DeviceStorage

a dedicated page here:

https://developer.mozilla.org/en-US/docs/Web/API/DeviceStorage/lowDiskSpace

and a note to the relevant release notes:

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Releases/2.5

Let me know if these look ok. Thanks!

Comment 16

[Scott Wu [:scottwu]]

I'm working on Bug 1208892, which should display warning messages in settings when app storage is almost full.

I filled up my device to < 5MB left and found a strange issue:

When I tried to inspect navigator.getDeviceStorage('apps').lowDiskSpace from WebIDE console, in Settings it shows false (incorrectly), but in System it shows true (correctly).

Wonder if it's a bug in the API?

Comment 17

[Scott Wu [:scottwu]]

Oops I meant Bug 1208893

Comment 18

[Gabriele Svelto [:gsvelto]]

Sorry for the delay, it was due to the branch date for 2.5. I'm now looking into this.

Comment 19

[Gabriele Svelto [:gsvelto]]

OK, I've figured out where the problem is and will open a new bug to fix it: I'm not doing IPC correctly so the low-space notification event I'm listening to is handled only in the main process. In content processes the message is never sent (there's no disk space watcher!) and thus the field is never set to true.