NativeFileSystem APIs pass inconsistent types to error handlers

[peterflynn]

The error callbacks passed to various NativeFileSystem APIs are sometimes passed an integer error code, vs. sometimes passed a FileError object containing such an error code. Sometimes even different codepaths through the same method are inconsistent here.

The spec is unambiguous on which one is correct: "the err argument to the ErrorCallback MUST be a DOMError object..."

• FileEntry.createWriter(), Entry.getMetadata(), and requestNativeFileSystem() all pass a number insteead of a FileError
• DirectoryEntry.getFile() and getDirectory() pass a number _or_a FileError, depending on case
• DirectoryReader.readEntries() is similarly inconsistent
• showOpenDialog(), albeit nonstandard, passes a number

[pthiess]

Reviewed

[jbalsas]

Hi,

I was looking at this, and it looks like the error callbacks are all being called with a FileError. What seems to be wrong is the jsdoc in _nativeToFileError:

/**
 * Converts a brackets.fs.ERR_* error code to a FileError.* error code
 * @param {number} nativeErr
 * @return {number}
 */
 _nativeToFileError: function (nativeErr) { 
   var error;
   [...]
   return new NativeFileSystem.FileError(error);   
 }

@peterflynn Is it possible that this caused the confussion, or am I missing something?

[jbalsas]

Another thing is that looking at the specs there are some difference between the draft originally used in the implementation of NativeFileSystem and the current one

In the old one, ErrorCallback was supposed to receive a FileError, but that interface seems to have been deprecated in the currents specs and it expects a DomError instead.

Mozilla already dropped FileError as of Firefox 13.0, though I haven't been able to find any information regarding plans for this on Chrome.

I guess this is one of the perks of implementing working drafts... should we update NativeFileSystem already to align with the new specifications?

[jasonsanjose]

FBNC @peterflynn

[peterflynn]

@jbalsas: sorry about my mistake in the original bug, and thanks for the cleanup in pull #2318!

@jasonsanjose: I noticed that resolveNativeFileSystemPath() doesn't follow Chema's new conventions, though. It calls the old conversion function (which no longer exists afaict) and doesn't new up a NativeFileError to wrap the error code in... Shall one of us fix that real quick before closing this?

[jbalsas]

@peterflynn About resolveNativeFileSystemPath, I also noticed it just now while updating the comments for #2063. I fixed it along with the last batch of changes, so you could pick it up from there. That's of course unless you'd want it right away, in case that pull request takes some time to get through.

[peterflynn]

Closing now that #2063 has also landed