902565 - this.done in cursor callback of naviagtor.getDeviceStorage("sdcard").enumerate is undefined

Status: RESOLVED WORKSFORME

(Core :: DOM: Device Interfaces, defect)

Description

[(not reading bugmail) Nick Desaulniers [:\n]]

example:

var sdcard = navigator.getDeviceStorage("sdcard");
var cursor = sdcard.enumerate();
cursor.onsuccess = function () {
  alert("this.done is: " + this.done); // this.done is: undefined
};

I modified the MDN page, so that should be reverted when fixed. [1]

[1] https://developer.mozilla.org/en-US/docs/Web/API/DeviceStorage.enumerate$history

Comment 1

[(not reading bugmail) Nick Desaulniers [:\n]]

Also note that I only tested this in 1.0.1.

Comment 2

[:Harald Kirschner :digitarald]

Even Gaia code doesn't use .done but checks if .result is set: https://github.com/mozilla-b2g/gaia/blob/v1-train/apps/communications/contacts/js/utilities/sdcard.js?source=c#L62

Also no sign of using .done in the device storage tests in Gaia: https://github.com/mozilla-b2g/gaia/blob/v1-train/test_apps/ds-test/js/ds-test.js

Comment 3

[Johan Nenzén]

Hi, I recently experienced the same problom when following code example found here: https://developer.mozilla.org/en-US/docs/WebAPI/Device_Storage?redirectlocale=en-US&redirectslug=WebAPI%2FDevice_Storage_API

the solution to that example is to check whether file is not null and put "this.continue()" inside the null check. Please update the example as long as the problem exists.

Comment 4

[David Bruant]

Needinfo'ing Mounir and Jonas to ask whether they'd be aware of a regression. Maybe it's just a documentation issue?

Comment 5

[(not reading bugmail) Nick Desaulniers [:\n]]

David, I don't think it's a documentation issue if it's not used that way in Gaia (as per Harald's research in Comment #2).

Comment 6

[Dave Hylands [:dhylands]]

Hmm.

I just tried this on master, and it seems to be working for me.

I modified ds-test.js around line 412 (in the onsuccess inside enumerateAll) so that it looked like:

  function onsuccess(e) {
    cursor.result = e.target.result;
    dump('onsuccess: this.done = ' + this.done + '\n');
    dump('onsuccess: ds_cursor.done = ' + ds_cursor.done + '\n');

and when calling enumerateAll I see the following in my logcat:

onsuccess: this.done = false
onsuccess: ds_cursor.done = false

for the cases when cursor.result is non-null and 

onsuccess: this.done = true
onsuccess: ds_cursor.done = true

when cursor.result is null.

I wasn't using .done in ds-test.js because I wasn't aware of it. I'll file a bug to update ds-test.js to use .done

Comment 7

[(not reading bugmail) Nick Desaulniers [:\n]]

Dave, can you verify this in 1.0.1 and 1.1?  It's an issue for developers if the API differs between versions and should be documented.

Comment 8

[Dave Hylands [:dhylands]]

I filed bug 905294 to rework ds-test to use .done.

Comment 9

[Dave Hylands [:dhylands]]

(In reply to Nick Desaulniers [:\n] from comment #7)
> Dave, can you verify this in 1.0.1 and 1.1?  It's an issue for developers if
> the API differs between versions and should be documented.

Sure will do.

Comment 10

[Johan Nenzén]

FYI, I was using Geekphone Device with Boot2Gecko 1.0.1.0-prerelease and the simulator running Boot2Gecko 1.1.0.0-prerelease. Same issue on both.

Comment 11

[Dave Hylands [:dhylands]]

(In reply to Nick Desaulniers [:\n] from comment #7)
> Dave, can you verify this in 1.0.1 and 1.1?  It's an issue for developers if
> the API differs between versions and should be documented.

It appears that bug 837865 modified DeviceStorage to use DOMCursor, and that particular bug only landed on master.

I verified that this.done was undefined on 1.1 (I didn't bother confirming 1.0.1 since it should be the same).

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So it sounds like this is working as "expected" and that .done isn't implemented for v1.0.1/v1.1 and will only be available in v1.2.

Certainly not great, but that's how it is with old code.

Is this blocking any particular apps?

Comment 13

[(not reading bugmail) Nick Desaulniers [:\n]]

No, I had written up a file browser app based on feedback and just had to work around it.  The documentation should probably be reverted back with a note.

Comment 14

[(not reading bugmail) Nick Desaulniers [:\n]]

instead, one may put the call to this.continue() within a conditional check for this.result:

https://github.com/nickdesaulniers/filebrowser/blob/eb49f30954c0d69c355bfab3bdbde453d5451894/myscript.js#L22-L45

Comment 15

[Dave Hylands [:dhylands]]

In our code reviews we'd say that you should do this instead:

if (!this.result) {
    return;
}

and then unindent the rest of the function by one level

You shouldn't call this.continue once this.result returns null (since you're finished, it doesn't make sense to call continue).

Alternatively you could do:

if (!this.result) {
    this.done();
    return;
}

Comment 16

[(not reading bugmail) Nick Desaulniers [:\n]]

You also shouldn't invoke this.done if it's undefined. :P

Comment 17

[Dave Hylands [:dhylands]]

Marking this as resolved works for me, since the behaviour observed appears to be intended.

I also added dev-doc-needed so that we can document .done as a method to detect the end of the enumeration for FxOS 1.1 and newer.