Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

NativeFileError for error callbacks in NativeFileSystem (#2057) #2318

Merged
merged 7 commits into from
Dec 11, 2012
Merged

NativeFileError for error callbacks in NativeFileSystem (#2057) #2318

merged 7 commits into from
Dec 11, 2012

Conversation

jbalsas
Copy link
Contributor

@jbalsas jbalsas commented Dec 9, 2012

This is a possible fix for #2057. It introduces a new NativeFileError implementing DOMError to substitute the current FileError as the specs dictate.

Important changes

  • There's no need anymore for the FileError polyfill
  • NativeFileSystem.FileError is moved to its own module NativeFileError. I thougth it would make more sense, and avoids dependencies with NativeFileSystem when only wanting to check error name values.
  • _nativeToFileError has been modified to just convert a brackets.fs error code to a NativeFileError error name. Hopefully this clears out any confusion in NativeFileSystem by making explicit all the error constructors in the same way.
  • DOMError has a name attribute where FileError had code.

Notes

  • There's no longer a dependency with the browser implementation of FileError, nor the need to use a polyfill for the in-browser branch.
  • Errors that don't have a custom message are more descriptive when printed on dialog errors (i.e. instead of showing error.4 you now see something like error. NotReadableError)
  • The attribute change between DOMError and FileError may break error handling in existing extensions. We could add a code attribute in our NativeFileError class to ensure backward compatibility. I've done a quick search though, and only three extensions seem to be using error.code right now.
  • I've run all the unit tests and smoke tests and everything seems to be working fine so I hope I didn't miss anything, but this change affects several files, so it could be a good idea to split it into several pull requests if you don't feel it's safe enough.

@ghost ghost assigned jasonsanjose Dec 10, 2012
* @return {number}
* Converts a brackets.fs.ERR_* error code to a NativeFileError.* error name
* @param {!number} nativeErr A brackets.fs error code
* @return {string} An error name out of the possible NativeFileError.* names
*/
_nativeToFileError: function (nativeErr) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of this function is a bit misleading now. How about _fsErrorToDOMErrorName?

@jasonsanjose
Copy link
Member

Initial review complete.

What were the 3 extensions that use error.code? I'd like to contact the authors when this change lands.

@jbalsas
Copy link
Contributor Author

jbalsas commented Dec 10, 2012

I've updated the name and its references and pushed the changes.

The three extensions I've identified are:

@jasonsanjose
Copy link
Member

@jbalsas Need to merge with master

…ystem-domerror

Conflicts:
	src/file/NativeFileSystem.js
@jbalsas
Copy link
Contributor Author

jbalsas commented Dec 11, 2012

Done! ;)

@jasonsanjose
Copy link
Member

Confirmed unit tests are passing on mac. Merging.

jasonsanjose added a commit that referenced this pull request Dec 11, 2012
NativeFileError for error callbacks in NativeFileSystem (#2057)
@jasonsanjose jasonsanjose merged commit 41646b8 into adobe:master Dec 11, 2012
@jasonsanjose
Copy link
Member

@davidderaedt, @cfjedimaster and @jrowny: Please see the changes described above.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants