Closed Bug 1204618 Opened 8 years ago Closed 8 years ago

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


(Core :: DOM: Device Interfaces, defect)

Not set



Tracking Status
firefox43 --- fixed


(Reporter: gsvelto, Assigned: gsvelto)



(Keywords: dev-doc-complete)


(1 file, 2 obsolete files)

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.
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.
Attachment #8660903 - Flags: feedback?(dhylands)
Comment on attachment 8660903 [details] [diff] [review]

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.
Attachment #8660903 - Flags: feedback?(dhylands) → feedback+
Assignee: nobody → gsvelto
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.
Attachment #8660903 - Attachment is obsolete: true
Attachment #8661200 - Flags: review?(dhylands)
Attachment #8661200 - Flags: review?(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.

Attachment #8661200 - Flags: review?(bzbarsky) → review+
Attachment #8661200 - Flags: review?(dhylands) → review+
Thanks to both for the quick reviews! Updated patch with nits addressed, the try run is here:
Attachment #8661200 - Attachment is obsolete: true
Attachment #8662017 - Flags: review+
After retriggering a lot of jobs with intermittent failures I've finally got a green try run:

Time to land
Keywords: checkin-needed
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
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.
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?
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.
I'd think "diskSpaceStatus" would make more sense there.
why not !
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.
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?
Oops I meant Bug 1208893
Flags: needinfo?(gsvelto)
Sorry for the delay, it was due to the branch date for 2.5. I'm now looking into this.
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.
Flags: needinfo?(gsvelto)
Blocks: 1221086
You need to log in before you can comment on or make changes to this bug.