Create a device storage api which only returns media state (ready, unavailable, etc..)

RESOLVED FIXED in Firefox 21

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: lightsofapollo, Assigned: dougt)

Tracking

({dev-doc-needed, perf})

unspecified
mozilla21
dev-doc-needed, perf
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:tef+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 fixed, b2g18-v1.0.1 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Essentially we want to expose the volume state without incurring the cost of a full .stat call (which can take a very long time 1-2 seconds or more on large datasets).
(Reporter)

Comment 1

5 years ago
Currently we use stat for this same purposes which is taking somewhere between 1 and 2 seconds... From what I understood this should be a very fast operation by comparison since we really only care about state and nothing else...
blocking-b2g: --- → leo?
Keywords: perf
(Assignee)

Comment 2

5 years ago
looking at what gaia does, i think it is best that we factor out stat() into 3 separate calls:


    nsIDOMDOMRequest freeSpace();
      e.target.result == free space for the given device storage object


    nsIDOMDOMRequest usedSpace();
      e.target.result == used space for the given device storage object

    nsIDOMDOMRequest available();
      e.target.result == same was what stat() state did.
(Assignee)

Comment 3

5 years ago
I should note, that gaia will have to make some changes to support this.  Most changes are trivial.
(Reporter)

Comment 4

5 years ago
( IMO ) this api makes a lot of sense to me... it makes it really obvious what each method is for as well.
(Assignee)

Comment 5

5 years ago
Created attachment 706434 [details] [diff] [review]
patch 1/2 v.1
Attachment #706434 - Flags: review?(jonas)
(Assignee)

Comment 6

5 years ago
Created attachment 706435 [details] [diff] [review]
patch 2/2 v.1
Attachment #706435 - Flags: review?(jonas)
(Assignee)

Comment 7

5 years ago
oh, and uuid bump needed.
(Assignee)

Comment 8

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=16df131035bf
Comment on attachment 706434 [details] [diff] [review]
patch 1/2 v.1

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

r=me with comments addressed.

::: dom/devicestorage/DeviceStorageRequestParent.cpp
@@ +626,5 @@
> +  }
> +#endif
> +
> +  AvailableStorageResponse response(state);
> +  unused <<  mParent->Send__delete__(mParent, response);

Is unused << really necessary here?  If so fix the double space.

::: dom/devicestorage/DeviceStorageRequestParent.h
@@ +175,5 @@
>        nsRefPtr<DeviceStorageFile> mFile;
>        nsString mPath;
>    };
>  
> + class PostStatResultEvent : public CancelableRunnable

Er, you should fix this by indenting the brace one more space, not removing a space from here, no?

@@ +188,5 @@
>        int64_t mFreeBytes, mTotalBytes;
> + };
> +
> + class PostAvailableResultEvent : public CancelableRunnable
> + {

Dude two spaces.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +2087,5 @@
> +    return NS_ERROR_UNEXPECTED;
> +  }
> +
> +  nsRefPtr<DOMRequest> request = new DOMRequest(win);
> +  NS_ADDREF(*aRetval = request);

request.forget(&aRetval)?

::: dom/devicestorage/test/test_available.html
@@ +16,5 @@
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=834595">Mozilla Bug 834595</a>
> +<p id="display"></p>
> +<div id="content" style="display: none">
> +  

Nit: whitespace at EOL

@@ +24,5 @@
> +
> +devicestorage_setup();
> +
> +function availableSuccess(e) {
> +  ok(e.target.result != null, "result should not be null");

isnot(e.target.result, null, ...);

@@ +34,5 @@
> +  devicestorage_cleanup();
> +}
> +
> +var storage = navigator.getDeviceStorage("pictures");
> +ok(navigator.getDeviceStorage, "Should have getDeviceStorage");

If this is not supposed to be ok(storage, "Should have storage"); then move it above the function call.  No point in checking the function is there *after* you call it.

::: dom/interfaces/devicestorage/nsIDOMDeviceStorage.idl
@@ +39,5 @@
>      nsIDOMDeviceStorageCursor enumerateEditable([optional] in jsval aName, /* DeviceStorageEnumerationParameters */ [optional] in jsval options);
>  
>      nsIDOMDOMRequest stat();
>  
> +    nsIDOMDOMRequest available();

needs uuid change.
Attachment #706434 - Flags: review+
Comment on attachment 706435 [details] [diff] [review]
patch 2/2 v.1

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

::: dom/interfaces/devicestorage/nsIDOMDeviceStorage.idl
@@ +39,5 @@
>      nsIDOMDeviceStorageCursor enumerateEditable([optional] in jsval aName, /* DeviceStorageEnumerationParameters */ [optional] in jsval options);
>  
> +    nsIDOMDOMRequest freeSpace();
> +
> +    nsIDOMDOMRequest usedSpace();

IID change.
Attachment #706435 - Flags: review+
Attachment #706434 - Flags: review?(jonas) → superreview+
Attachment #706435 - Flags: review?(jonas) → superreview+
In speaking with dougt, this patch reduces app start-up time in some cases by at least 2 seconds. App start-up is rather large concern right now so I'm noming for tef?.

Doug is confident that these patches are ready but there is likely to be fallout in several areas of Gaia. (Settings, Music, Video, ...) James is working on testing this fix with Gaia in order to enumerate the breakage. Once we understand the sources of breakage we need to coordinate the Gaia fixes so that they land along with this change. That's the long way of saying that we should not land this fix on b2g18 until the corresponding Gaia changes are ready.
blocking-b2g: leo? → tef?
(Assignee)

Comment 12

5 years ago
Created attachment 706503 [details] [diff] [review]
combined patch.
Attachment #706434 - Attachment is obsolete: true
Attachment #706435 - Attachment is obsolete: true
Attachment #706503 - Flags: review+
It looks like these are the files in gaia that use stat().

find . -name '*.js' | xargs grep -l 'stat(' 

./apps/camera/js/camera.js
./apps/email/js/ext/gaia-email-opt.js
./apps/gallery/js/gallery.js
./apps/settings/js/media_storage.js
./apps/settings/js/utils.js
./apps/system/camera/js/camera.js
./apps/system/js/bluetooth_transfer.js
./apps/system/js/screenshot.js
./shared/js/mediadb.js
Gaia WIP here: https://github.com/mozilla-b2g/gaia/pull/7809

So far there is just one commit in there, fixing the camera app.  Untested, since I still don't have gecko built.
Okay, a full set of gaia-side fixes are in the pull request linked above. I've still got to test, though.  Email didn't need to be patched... it was calling some other stat function.
First round of tests with an sdcard with enough free space on it:

camera: can take photos and record videos

gallery: can browse photos and edit photos

settings: media storage and device storage panels seem to display reasonable numbers
When ums is enabled media storage does not display used space numbers, as expected. And when ums is disabled again, the numbers appear, as expected.

screenshots: can take screenshots normally

bluetooth: can receive files normally

lockscreen camera: can take pictures and record videos
Second round of tests. This time with an sdcard with only 1mb free space:

lockscreen camera: shows "not enough space" overlay, as expected

regular camera: ditto

gallery: can browse and view photos. Can edit some photos, but then as space is used up, it displays a "can't edit photos card is full" message, as expected.

screenshots: can take about 4 screenshots, then I get a "not enough space" message until I delete screenshots from gallery.  as expected.

bluetooth transfer: correctly displays a "card is full" message if there isn't enough space to receive the file.  (It doesn't seem to tell the sender that the transfer has been cancelled, but maybe bluetooth doesn't support that)

settings: correctly displays that there is only about 1mb of space left on the card
Third set of tests, this time with the card removed (and then inserted, or inserted and hten removed)

lockscreen camera: correctly displays a no card message, and removes it when the card is inserted

regular camera: ditto

gallery: displays no card message, then starts running on insert

screenshots: correctly displays no card message

bluetooth: correctly displays no card message

settings: media storage says not found, panel is disabled. When card is inserted, free space is displayed and panel is enabled.
Final round of tests: with ums enabled

camera: unplug phone message comes and goes as phone is plugged in and unplugged.

gallery: ditto

screenshots: displays correct message

bluetooth: displays appropriate message

settings: I tested this case in the first round of tests. works.

lockscreen camera: if the app is launched while UMS is in progress, it displays an appropriate overlay. And if unplugged it starts working.  But then when plugged in again, it keeps working.  The UMS session isn't being resumed.  That is probably actually a security feature, so I'll call that good.
Created attachment 706692 [details]
Gaia side patch to use the new API

This is the Gaia half of this bug.  Tested as described in the comments above.

Doug: as the author of the new API you may be the best suited to review the gaia side. But in case you don't want to, I've also put James down for r?. He's on a plane right now and won't get to it 'till Monday, though.

James: I have not tested how much this speeds up the media apps, but I'm sure you will want to measure that.
Attachment #706692 - Flags: review?(jlal)
Attachment #706692 - Flags: review?(doug.turner)
(Reporter)

Comment 21

5 years ago
Comment on attachment 706692 [details]
Gaia side patch to use the new API

Address/Argue my settings comment in bugzilla then r+
Attachment #706692 - Flags: review?(jlal) → review+
(Assignee)

Comment 22

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3b2be18c306
(In reply to James Lal [:lightsofapollo] from comment #21)
> Comment on attachment 706692 [details]
> Gaia side patch to use the new API
> 
> Address/Argue my settings comment in bugzilla then r+

I chose to argue on github instead of address. So for the reasons listed there, I say we just land this as is.

Doug, you can be the final arbiter of this, since I assume you will be the one to push the gaia and gecko pieces together at the same time.
(Assignee)

Comment 24

5 years ago
This should land.  waiting on tef+
https://hg.mozilla.org/mozilla-central/rev/c3b2be18c306
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Let's make sure we coordinate the landings in gecko and gaia so we reduce breakage.
blocking-b2g: tef? → tef+
(Assignee)

Comment 27

5 years ago
 https://hg.mozilla.org/releases/mozilla-b2g18/rev/7dfa01512f88

I would like this to get onto the 1.0.0 release.  Andrew, how do we do that?
pushed to gaia master: 

https://github.com/mozilla-b2g/gaia/commit/9f26da91f91850768c6d5ffbe3ace56380411d2e
pushed to gaia v1.0.0:

https://github.com/mozilla-b2g/gaia/commit/e0dbfd08cc59ba6bc22e7f60f9fadd36bb3526f9
pushed to gaia v1-train:

https://github.com/mozilla-b2g/gaia/commit/393e375d7aa286e981f1c4e169b908c1c90832cc

Updated

5 years ago
Keywords: dev-doc-needed
I'm pretty sure this broke us, here: bug 835655.
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/ace4596d2b67
status-b2g18: --- → fixed
status-firefox19: --- → wontfix
status-firefox20: --- → wontfix
status-firefox21: --- → fixed
(Assignee)

Updated

5 years ago
Attachment #706692 - Flags: review?(doug.turner)
Depends on: 835918
status-b2g18-v1.0.0: --- → fixed
status-b2g18-v1.0.1: --- → fixed
You need to log in before you can comment on or make changes to this bug.