Closed Bug 754350 Opened 9 years ago Closed 8 years ago

Device Storage - Clean up error strings

Categories

(Core :: DOM: Device Interfaces, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
blocking-basecamp -
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: dougt, Assigned: dougt)

Details

Attachments

(1 file, 2 obsolete files)

Followup from bug 717103 - I am leaving the error strings for debugging for now.  We should clean up up and subclass DOMError (or something)
Attached file idea sketch (obsolete) —
I don't expect this to be exactly what was desired, but this was my first shot at creating something that might do most of what it needs to.  What other capabilities should this have?
Attachment #642140 - Flags: feedback?(doug.turner)
Attachment #642140 - Attachment is patch: false
OS: Mac OS X → All
Hardware: x86 → All
Comment on attachment 642140 [details]
idea sketch

does this work?
I haven't tried to plug it into the code or anything, I just wanted to get an idea of what was being looked for.  If this looks okay, I will try to plug it in and make it work.
OS: All → Mac OS X
Hardware: All → x86
mName should be set to one of the strings from

http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#error-names-0

or

http://dev.w3.org/2006/webapi/FileAPI/#dfn-error-codes

Or if you feel like non of them are matching we can make up new ones, but should follow the same pattern.

So for DEV_STORE_FILE_DOES_NOT_EXIST .name should be set to "NotFoundError"
For DEV_STORE_ERROR_PERMISSION_DENIED .name should be set to "SecurityError"
For DEV_STORE_ERROR_UNKNOWN .name should be set to "UnknownError"
etc.

For DEV_STORE_ERROR_NON_STRING_TYPE_UNSUPPORTED we should simply throw a TypeError exception, no need to create a DOMError at all. You can throw a TypeError by returning NS_ERROR_TYPE_ERR as nsresult.
I also don't think there's a need for .type if it doesn't contain any more information than what can be deduced from .name.

And is DeviceStorageFile a scriptable interface??
DeviceStorageFile is not scriptable.  it is used only by our implementation.

you can remove all of the path handling in nsDeviceStorageError.  That was just for me.  Maybe make it #ifdef DEBUG.
Attached patch idea sketch v2 (obsolete) — Splinter Review
Attachment #642140 - Attachment is obsolete: true
Attachment #642140 - Flags: feedback?(doug.turner)
So if this is all that we are planning on exposing, then you could just create an existing DOMError. It already supports setting arbitrary values as .name.

The only reason we would need to subclass is if we want to expose additional information than the error type (which is exposed as .name). By subclassing we can add additional properties which would contain this additional information.
Got an "Unknown" error while trying to write to a file on the sdcard which was already present. 

Possibly we would want a separate error case for this scenario.

I would venture that we should check if a file exists prior to attempting to write, and if it does exist we through "already exists" error or something like that.
throw* 

ack.. no excuse for that one.
dclarke, this isn't the bug you're looking for.  please file a separate one and assign it to me.  if it is blocking your work, please mark it as such.
Attached patch patch v.1Splinter Review
Attachment #643666 - Attachment is obsolete: true
Attachment #670539 - Flags: review?(jonas)
noming - we really should clean up the error names so apps can depend on them.
blocking-basecamp: --- → ?
One of the patches in this push caused Windows mochitest-2 failures. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/91616129b892

https://tbpl.mozilla.org/php/getParsedLog.php?id=16083979&tree=Mozilla-Inbound

2054 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/devicestorage/test/test_dotdot.html | Error must be SecurityError
On windows, we were returning a type error instead of a security error.  I fixed this, and pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=ea3a9ef38d71
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.