If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Device Storage - Clean up error strings

RESOLVED FIXED

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dougt, Assigned: dougt)

Tracking

unspecified
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(blocking-basecamp:-, firefox18 fixed, firefox19 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Followup from bug 717103 - I am leaving the error strings for debugging for now.  We should clean up up and subclass DOMError (or something)

Comment 1

5 years ago
Created attachment 642140 [details]
idea sketch

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)

Updated

5 years ago
Attachment #642140 - Attachment is patch: false

Updated

5 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 2

5 years ago
Comment on attachment 642140 [details]
idea sketch

does this work?

Comment 3

5 years ago
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??
(Assignee)

Comment 6

5 years ago
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.

Comment 7

5 years ago
Created attachment 643666 [details] [diff] [review]
idea sketch v2
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.
(Assignee)

Comment 11

5 years ago
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.
(Assignee)

Comment 12

5 years ago
Created attachment 670539 [details] [diff] [review]
patch v.1
Attachment #643666 - Attachment is obsolete: true
Attachment #670539 - Flags: review?(jonas)
(Assignee)

Comment 13

5 years ago
noming - we really should clean up the error names so apps can depend on them.
blocking-basecamp: --- → ?
Attachment #670539 - Flags: review?(jonas) → review+
blocking-basecamp: ? → -
Attachment #670539 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 14

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/302400cbd111
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
(Assignee)

Comment 16

5 years ago
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
(Assignee)

Comment 17

5 years ago
https://hg.mozilla.org/mozilla-central/rev/838eee8853ca
https://hg.mozilla.org/releases/mozilla-aurora/rev/3ab210d3f02a
status-firefox18: --- → fixed
status-firefox19: --- → fixed
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.