Closed Bug 834595 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

Product:
Core
Component:
DOM: Device Interfaces
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla21
Project Flags:
blocking-b2g tef+
Tracking Flags:
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- fixed
b2g18-v1.0.1 --- fixed

People

(Reporter: jlal, Assigned: dougt)

References

Details

(Keywords: dev-doc-needed, perf)

Attachments

(2 files, 2 obsolete files)

combined patch.
46.26 KB, patch
dougt
: review+
Details | Diff | Splinter Review
Gaia side patch to use the new API
108 bytes, text/html
jlal
: review+
Details 
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).
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
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.
I should note, that gaia will have to make some changes to support this.  Most changes are trivial.
( IMO ) this api makes a lot of sense to me... it makes it really obvious what each method is for as well.
Attached patch patch 1/2 v.1 (obsolete) — — Splinter Review 

        Attachment #706434 -
        Flags: review?(jonas)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch 2/2 v.1 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #706435 -
        Flags: review?(jonas)

    
    

    
  
oh, and uuid bump needed.

    
    

    
  
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?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        combined patch.
        — 
          — Splinter Review
      

    


  

        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.

    
    

    
  


  
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)

    
    

    
  
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+

    
    

    
  
(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.

    
    

    
  
This should land.  waiting on tef+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/c3b2be18c306
Status: NEW → RESOLVED
Closed: 11 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+

    
    

    
  
 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?

    
  
Keywords: dev-doc-needed

    
    

    
  
I'm pretty sure this broke us, here: bug 835655.

    
  

        Attachment #706692 -
        Flags: review?(doug.turner)
Depends on: 835918

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: