861921 - Notify the user about the low free storage situation

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

Once we get notifications from the fanotify based component being build in bug 861898, we need to let the user know about the low storage situation.

Comment 1

[Alex Keybl [:akeybl]]

Bug 853350 already has a proposed UX spec. Should we dupe to that?

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

I don't think we should dupe to Bug 853350. That bug will track the creation of a device space watcher. I'll move the proposed UX spec to this bug.

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Low Storage UX specs, v2

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Josh, thanks for the UX spec proposal, a few comments about it:

1. It would be great if we could also add a persistent notification about the low storage condition while it exists. Something like a notification bar icon that reminds the user about the current state of her device.

2. The first banner for the "Device Storage Warning" page has the same text as the second one. Is it intentional?

3. Answering to your question about the distinction between app types, we can probably do that, but I don't think we should. The idea is to avoid installing *any* kind of apps. Hosted apps also use device storage (for the app manifest for example). In any case, the notification for not enough device storage while installing apps is already implemented and working.

Comment 5

[Wayne Chang [:wchang]]

Triage 4/17 agrees tef+ per 853350

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Attachment: B2G part WIP

Comment 7

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Gaia part - PR link

Pointer to Github pull-request

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Can't we do that via the app: part of the device storage API directly in Gaia. Seems like this is what http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/FreeSpaceWatcher.jsm is doing anyway.

Comment 9

[[:fabrice] Fabrice Desré]

We don't want to poll continuously, that's a waste of resource compared to the real-time info we get from the kernel. Also, doing it in gaia would mean sending back the notification from gaia to gecko which looks like the wrong way.

We used the timer + device storage in FreeSpaceWatcher.jsm because the monitoring there is short-lived. We'll convert it to use the new API in a followup.

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

Indeed. I've filed Bug 863596

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Fabrice Desré [:fabrice] from comment #9)
> We don't want to poll continuously, that's a waste of resource compared to
> the real-time info we get from the kernel. Also, doing it in gaia would mean
> sending back the notification from gaia to gecko which looks like the wrong
> way.

I was not suggesting to move the apps code in Gaia but to implement this feature in Gaia directly.

> 
> We used the timer + device storage in FreeSpaceWatcher.jsm because the
> monitoring there is short-lived. We'll convert it to use the new API in a
> followup.

I didn't know about this new API, it sounds better obviously. BUt instead of using our dirty little mozChromeEvent / mozContentEvent, I wonder if that would be better to fire an event on the deviceStorage returned by a call to getDeviceStorage('apps');

The low level code can listen for the observers messages.

Comment 12

[Josh Carpenter [:jcarpenter]]

Spoke about this with Fernando and Fabrice and we're aligned for 1.0.1 and 1.1 for UX. We'll use a single notification when the available device storage drops below the low storage threshold. Notification copy should be: 

Line 1: "Device space low:" 
Line 2: "X MB remaining"

Where X is the amount remaining. 

Notification is then pinned inside the Notification Tray, to the top, until the condition is cleared. Ideally "X MB remaining" will continue to update, but if that is not possible, let's remove it, since we do not want it to become inaccurate over time.

Clicking the notification should take the user to Settings > Device Storage. Which is admittedly not very helpful in v1.0.1 and 1.1, but is probably the best we can do. We're open to better options, however.

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #11) 
> I didn't know about this new API, it sounds better obviously. BUt instead of
> using our dirty little mozChromeEvent / mozContentEvent, I wonder if that
> would be better to fire an event on the deviceStorage returned by a call to
> getDeviceStorage('apps');
> 
> The low level code can listen for the observers messages.

We can do that.

I don't have an strong opinion about how to expose this event. We could:

A. Use the current "onchange" event [1], adding extra information about the free space and a "lowStorage" flag.

B. Use a new "onlowstorage" event that would include the remaining free space and a flag indicating if the notification is for a low storage situation or because the device has been recovered to a healthy storage situation.

C. Use two events "onlowstorage"/"onfreestorage" containing the free space in both cases.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/interfaces/devicestorage/nsIDOMDeviceStorage.idl#22

Comment 14

[Doug Turner (:dougt)]

Does the event really need to show how much space exists in a given mount point?  I am not sure that really means anything to most users.  Josh?



onchange is probably the right thing.  Currently we have the following:

  readonly attribute DOMString path;
  readonly attribute DOMString reason;

Maybe we just add a new reason "low-disk-space" -or- something similar sounding.  We should probably also dispatch another onchange when the space is 'okay' again.

Does this make sense?

Comment 15

[[:fabrice] Fabrice Desré]

(In reply to Doug Turner (:dougt) from comment #14)
> Does the event really need to show how much space exists in a given mount
> point?  I am not sure that really means anything to most users.  Josh?

Right now this is the only way a user can know how much space he cleared by uninstalling an application. That's not ideal but we can't do better for v1.

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

Thanks Doug!

I guess we can also use nsIDOMDeviceStorage.freeSpace [1] instead of including that information within the event.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/interfaces/devicestorage/nsIDOMDeviceStorage.idl#41

Comment 17

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Gecko part - v1

This patch triggers an 'onchange' event with "low-disk-space"/"available-disk-space" as reason and with an empty path.

I'll be writing some tests for it asap.

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 738731 [details]
Gaia part - PR link

I've just updated the Gaia PR. Etienne, could you provide some feedback about it, please? It is just a WIP, but it is currently working on my tests.

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

Josh, do you think that we'd need an icon for the notification or the message is enough?

Comment 20

[Chris Lee [:clee]]

(In reply to Josh Carpenter [:jcarpenter] from comment #12)
> Line 1: "Device space low:" 
> Line 2: "X MB remaining"
> 
> Where X is the amount remaining. 

We could just say:

Line 2: "< X MB remaining" OR "Less than X MB remaining"

This wouldn't require the remaining storage amount to be updated.  

As for an icon, if we have a 'warning-like' icon already today, maybe we could re-use that?

I'll defer to Josh/Patryk on what is available already.

Comment 21

[Etienne Segonzac (:etienne)]

Comment on attachment 738731 [details]
Gaia part - PR link

I'll do some preemptive nit picking on github but this looks good!

Comment 22

[Dave Hylands [:dhylands]]

Comment on attachment 741422 [details] [diff] [review]
Gecko part - v1

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

r=me if you fixup the location that Add/RemoveObserver is called for dsik-space-watcher

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +2449,5 @@
>  
>      DeviceStorageFile* file = static_cast<DeviceStorageFile*>(aSubject);
>      Notify(NS_ConvertUTF16toUTF8(aData).get(), file);
>      return NS_OK;
> +  } else if (!strcmp(aTopic, "disk-space-watcher")) {

Since device storage primarily deals with media storage, and this is fo app storage, i think it would be worthwhile to add a comment that indicates that these messages are primarily for app storage.

@@ +2562,5 @@
>                                                       win, mPrincipal, dsf, request, this, aListener);
>    NS_DispatchToMainThread(r);
> +
> +  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +  obs->AddObserver(this, "disk-space-watcher", false);

This feels like its in the wrong place to be registering this observer. Shouldn't the disk-space-watcher observer be added in nsDOMDeviceStorage::SetRootDirectoryForType and removed in nsDOMDeviceStorage::Shutdown?

At least that's where the other observers are registered.

Comment 23

[Fernando Jiménez Moreno [:ferjm]]

Etienne, Dave, thanks for your feedback!

Etienne, I've just updated the PR with some tests and modifications. I'd like to review it myself before asking for your review though.

Comment 24

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 738731 [details]
Gaia part - PR link

Stas, could you review the l10n changes, please?

Comment 25

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Gecko part - v2

This patch addresses Dave's feedback from comment 22.

r+ from comment 22.

Comment 26

[Staś Małolepszy :stas]

Comment on attachment 738731 [details]
Gaia part - PR link

Please move the size units into the localization file.  You can use existing strings (not sure why we need them defined in two places):

https://github.com/mozilla-b2g/gaia/blob/8701166b9864e2f369c022653826cd6da00e2e12/apps/system/locales/system.en-US.properties#L68

or 

https://github.com/mozilla-b2g/gaia/blob/8701166b9864e2f369c022653826cd6da00e2e12/apps/system/locales/system.en-US.properties#L206

("bytes" is problematic as it should have proper plural form;  file a new bug to get it fixed?)


 +free-space={{freeSpace}} remaining 

This should use plurals because some languages might require to accord the adjective with the number, too.  See https://wiki.mozilla.org/L10n:B2G/Developers#Plurals

Lastly, please attach your next patch to this bug as an actual text attachment, not a link to the pull request.  It's easier for me to just use bugzilla's splinter to comment on code and I like the fact that all discussion about code stays in bugzilla.  Thank you.

Comment 27

[Fernando Jiménez Moreno [:ferjm]]

Thanks for the feedback Stas!

(In reply to Staś Małolepszy :stas (needinfo along with cc, please; if you want an r+ from me, attach your patch to the bug; I don't review pull requests) from comment #26)
> Lastly, please attach your next patch to this bug as an actual text
> attachment, not a link to the pull request.  It's easier for me to just use
> bugzilla's splinter to comment on code and I like the fact that all
> discussion about code stays in bugzilla.  Thank you.

Sorry about that, I wasn't aware of your preference. Pull requests (and its corresponding link attached to the bug) are the format requested for Gaia patches. I'll attach both, a text attachment and a PR link, next time so everyone reviewing this code can take a look at his preferred format.

Comment 28

[Josh Carpenter [:jcarpenter]]

(In reply to Chris Lee [:clee] from comment #20)
> (In reply to Josh Carpenter [:jcarpenter] from comment #12)
> > Line 1: "Device space low:" 
> > Line 2: "X MB remaining"
> > 
> > Where X is the amount remaining. 
> 
> We could just say:
> 
> Line 2: "< X MB remaining" OR "Less than X MB remaining"
> 
> This wouldn't require the remaining storage amount to be updated.  
> 
> As for an icon, if we have a 'warning-like' icon already today, maybe we
> could re-use that?
> 
> I'll defer to Josh/Patryk on what is available already.

"Less than X MB remaining" makes sense. Good call. Re: an icon, I'll ping Patryk / Eric separately and see if they have anything.

Comment 29

[Eric Pang [:epang] UX]

Attachment: Device Storage Icon

Hi, these are the app permission icons we use for device storage so I think they will make sense here as well.  Let me know if they need any adjustments. Thanks!

Comment 30

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Gaia part

Stas, I think this patch addresses your suggestions. I've used the strings at [1] for byte units and used plurals for the "free-space" string.

I've also filed Bug 867180.

[1] https://github.com/mozilla-b2g/gaia/blob/8701166b9864e2f369c022653826cd6da00e2e12/apps/system/locales/system.en-US.properties#L206

Comment 31

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 738731 [details]
Gaia part - PR link

Etienne, I've updated the PR with the last changes. Could you take a look at it, please? Thanks!

Comment 32

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Screenshot of the notification

Eric, thanks for the icon. This is how it looks like in the notification area. Let me know if you want me to do any change, please.

Comment 33

[Etienne Segonzac (:etienne)]

Comment on attachment 738731 [details]
Gaia part - PR link

I wanted to nit pick a bit more but couldn't find anything :)

Comment 34

[Alex Keybl [:akeybl]]

Comment on attachment 743640 [details] [diff] [review]
Gaia part

Axel may be back in the office before Stas - moving the l10n review over to him.

Comment 35

[Axel Hecht [:Pike]]

Comment on attachment 743640 [details] [diff] [review]
Gaia part

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

I don't think patch works, and it has a bunch of l12y bugs. I also expect it to regress the crash UI due to changing whitespace there.

::: apps/system/js/storage_watcher.js
@@ +65,5 @@
> +    var notification;
> +    if (typeof space !== 'undefined') {
> +      notification = msg + ':\n' + this._('free-space', {value: space});
> +    } else {
> +      notification = msg + ':\n' + this._('unknown-free-space');

Don't hardcode strings, ":\n" should be part of the localizable content.

@@ +77,5 @@
> +
> +  updateAvailableSpace: function dsw_updateAvailableSpace(space) {
> +    this._availableSpace.innerHTML = (typeof space !== 'undefined') ?
> +                                     this._('free-space', {value: space}) :
> +                                     this._('unknown-free-space');

innerHTML really? That's insecure, and slow, can you get away with textContent?

@@ +132,5 @@
> +        req.onsuccess = function onsuccess() {
> +          var free;
> +          if (typeof req.result !== 'undefined') {
> +            var formatedSize = self.formatSize(req.result);
> +            free = formatedSize.size + ' ' + formatedSize.units;

Please don't hardcode ' '. There's an existing example on how to use units right, you should mirror what fileSize does.

# Localization note (fileSize*): The string is a float.
fileSize = {{size}} {{unit}}

Also, you're passing a string to something, and then use the plural form? How does that even work?

::: apps/system/locales/system.en-US.properties
@@ +185,4 @@
>  crash-reports-description-2=This may include things like open pages and apps, text typed into forms and the content of open messages, recent browsing history, or geolocation used by an open app.
>  # Localization note (crash-reports-description-3-*): These strings are a paragraph, with a "privacy policy"
>  # link in the middle. Include trailing spaces as needed.
> +crash-reports-description-3-start=We use crash reports to try to fix problems and improve our products. We handle your information as we describe in our

The removal of end-of-line whitespace is likely bad for this UI.

@@ +310,5 @@
> +free-space[one]=Less than {{value}} remaining
> +free-space[two]=Less than {{value}} remaining
> +free-space[few]=Less than {{value}} remaining
> +free-space[many]=Less than {{value}} remaining
> +free-space[other]=Less than {{value}} remaining

Once this uses value and unit independently, there should be a comment on what the two are.

Comment 36

[Eric Pang [:epang] UX]

(In reply to Fernando Jiménez Moreno [:ferjm] (offline until 3rd of May) (use needinfo instead of CC, please) from comment #32)
> Created attachment 743644 [details]
> Screenshot of the notification
> 
> Eric, thanks for the icon. This is how it looks like in the notification
> area. Let me know if you want me to do any change, please.

Hi Fernando,

Thanks for implementing this!  I misunderstood where the icon was being used (so I made it the wrong size and style).  I'll attach the correct ones and if you can use them as a replacement that would be great.  Can you flag me for info once they are swapped out so I take a look and make sure they look right? Thanks!

Comment 37

[Eric Pang [:epang] UX]

Attachment: Device Storage Icon

Hi Fernando, here are the re-sized and restyled low storage icons. Thanks!

Comment 38

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Gaia part - v2

Thanks for the feedback Axel. This patch tries to address all your comments. Please, let me know if I missed anything.

Comment 39

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Screenshot

Thanks for the new icons Eric! This is how it looks like now.

Comment 40

[Axel Hecht [:Pike]]

Comment on attachment 744589 [details] [diff] [review]
Gaia part - v2

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

I'm still confused when the strings are used separately and when combined, and what that means.

I found a technical nit below, but this should be OK string wise.

If possible, avoid even the '+' of the two localized strings. But I really can't follow the code path to see if that is.

You could either turn the first string into taking the free-space as argument, or create a third combinator, {{message}}\n{{free_space}} or so.

Setting this as feedback+, I'm not really a reviewer at this point.

::: apps/system/js/storage_watcher.js
@@ +102,5 @@
> +    var i = 0;
> +    while (size >= 1024) {
> +      size /= 1024;
> +      ++i;
> +    }

you should terminate this loop so that i remains inside units. Yes, silly to expect more than 1000 YB free space, but still.

Comment 41

[Eric Pang [:epang] UX]

(In reply to Fernando Jiménez Moreno [:ferjm] (offline until 3rd of May) (use needinfo instead of CC, please) from comment #39)
> Created attachment 744591 [details]
> Screenshot
> 
> Thanks for the new icons Eric! This is how it looks like now.

No Problem, thanks for updating!  But the icon looks smaller then intended, can you check if it is being scaled down from 30x30px? Thanks!

Comment 42

[Chris Lee [:clee]]

UX team, is the text "Device space low" the right text?

Comment 43

[Eric Pang [:epang] UX]

(In reply to Chris Lee [:clee] from comment #42)
> UX team, is the text "Device space low" the right text?

I believe the text should be "Less than X MB remaining"
Josh can you confirm? Thanks!

Comment 44

[Stephany Wilkes]

Eric, that's more in line with existing strings (I just checked on the Mercurial string repo) and the copy style guide, so let's go with that. Clearing Josh so we can keep this moving. Thanks, Eric!

Comment 45

[Fernando Jiménez Moreno [:ferjm]]

As you can see in comment 12, Josh already defined the text to be shown as:

Line 1: "Device space low" 
Line 2: "X MB remaining"

After that, in comment 28, he agreed to change line 2 to:

"Less than X MB remaining"

And that is what it is currently implemented and what you can see in the attached screenshot.



Line 1: "Device space low."
Line 2: "Less than 194 MB remaining."(In reply to Eric Pang [:epang] from comment #41)

> (In reply to Fernando Jiménez Moreno [:ferjm] (offline until 3rd of May)
> (use needinfo instead of CC, please) from comment #39)
> > Created attachment 744591 [details]
> > Screenshot
> > 
> > Thanks for the new icons Eric! This is how it looks like now.
> 
> No Problem, thanks for updating!  But the icon looks smaller then intended,
> can you check if it is being scaled down from 30x30px? Thanks!

You are right! It was being scaled. Sorry about that. I am updating the PR and the screenshot.

Comment 46

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Screenshot

Comment 47

[Eric Pang [:epang] UX]

(In reply to Fernando Jiménez Moreno [:ferjm] (needinfo instead of CC, please) from comment #46)
> Created attachment 745032 [details]
> Screenshot

Thanks, sizing looks right now, but is it possible to shift the icon to the left by 3 pixels?  Sorry for all the adjustments, this will allow it to be centered under the download icon.  Let me know if it's doable, Thanks!

Comment 48

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Screenshot

Eric, I've just updated the PR and screenshot. Let me know if this looks correct now, please. Thanks!

Comment 49

[Eric Pang [:epang] UX]

(In reply to Fernando Jiménez Moreno [:ferjm] (needinfo instead of CC, please) from comment #48)
> Created attachment 746407 [details]
> Screenshot
> 
> Eric, I've just updated the PR and screenshot. Let me know if this looks
> correct now, please. Thanks!

This looks good now Fernando.  Thanks for all the updates!

Comment 50

[Johnny Stenback (:jst)]

The gecko patch here does not apply to current m-c. Fernando, can you update it and land this now that bug 853350 is landed on birch?

Comment 51

[Fernando Jiménez Moreno [:ferjm]]

http://hg.mozilla.org/projects/birch/rev/1d1c4c086287

Comment 52

[Fernando Jiménez Moreno [:ferjm]]

https://github.com/mozilla-b2g/gaia/commit/fad3da3881351a6a2b66fcddb2fe2dd0b72eddb2

Comment 53

[Alex Keybl [:akeybl]]

(In reply to Fernando Jiménez Moreno [:ferjm] (needinfo instead of CC, please) from comment #51)
> http://hg.mozilla.org/projects/birch/rev/1d1c4c086287

Let's not wait on full resolution to uplift given the deadline here - Ryan/John, can you uplift to v1.1/v1.0.1 please?

Comment 54

[Ryan VanderMeulen [:RyanVM]]

Please don't mess with bug resolutions. It doesn't help.

Comment 55

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Gecko part - b2g18

This patch applies in b2g18

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1d1c4c086287

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/ddcb5f19016a
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/a0b3e974355a

Leaving the status flags set to affected for the Gaia uplift.

Comment 58

[John Ford [:jhford] CET/CEST Berlin Time]

Commit fad3da3 does not apply cleanly to v1-train or v1.0.1, but both seem to have the same conflict.  This looks like a fairly simple resolution, but I'd prefer that someone more familiar with the code look at it.

To resolve this, please:

  cd gaia
  git checkout v1-train
  git cherry-pick -x -m1 fad3da3
  <resolve merge conflict>
  git add apps/system/index.html apps/system/locales/system.en-US.properties
  git commit
  git checkout v1.0.1
  git cherry-pick $(git rev-parse v1-train)

Comment 59

[Fernando Jiménez Moreno [:ferjm]]

v1.0.1: https://github.com/mozilla-b2g/gaia/commit/0d46a210900d7a27168de4cec6cafdd52d9d85bc
v1-train: https://github.com/mozilla-b2g/gaia/commit/ebe14f72aab0a306c33e4191a841e1c3c79e79af

Comment 60

[Michael Vines [:m1] [:evilmachines]]

Looks like the cherry-picks in comment 59 aren't quite right.  These two lines added to apps/system/index.html were not part of the original patch to gaia/master,

+    <!-- Shared responsive -->
+    <link rel="stylesheet" href="shared/style/responsive.css">

and cause bustage:

"build/webapp-zip.js:311: Error: Can't add inexistent file to zip : .../gaia/shared/style/responsive.css from: system.gaiamobile.org"

Comment 61

[[:fabrice] Fabrice Desré]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #60)
> Looks like the cherry-picks in comment 59 aren't quite right.  These two
> lines added to apps/system/index.html were not part of the original patch to
> gaia/master,
> 
> +    <!-- Shared responsive -->
> +    <link rel="stylesheet" href="shared/style/responsive.css">
> 
> and cause bustage:
> 
> "build/webapp-zip.js:311: Error: Can't add inexistent file to zip :
> .../gaia/shared/style/responsive.css from: system.gaiamobile.org"

This is filed in bug 871210

Comment 62

[Fernando Jiménez Moreno [:ferjm]]

Fixed in https://bugzilla.mozilla.org/show_bug.cgi?id=871210#c1

Sorry for the inconvenience.

Comment 63

[Jason Smith [:jsmith]]

Local storage & appcache tests repeatedly have shown this to work - both warning the user of the low storage when it happens along with having the persistent notification while you remain in low storage.