Bug 87276 – IndexedDB: Align codes and names for IDB-specific and DOM-specific errors/exceptions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alec Flett]

IndexedDB: Align codes and names for IDB-specific and DOM-specific errors/exceptions

[Alec Flett]

Created attachment 143595 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Alec Flett]

jsbell@, dgrogan@ mind taking a look - this patch is entirely about teasing out system (DOM) exception/error codes from IDB-specific exception codes, and providing a hook to let DOM exceptions have IDB-specific error messages:
http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#exceptions

All of the legacy constants hang off of IDBDatabaseException (as they do today)- and I'm introducing new constants because I've added name constants to avoid some of the issues we hit with the IDBCursor.NEXT/etc landing - in case someone does something like:

if (IDBDatabaseException[exception.name] == IDBDatabaseException.VERSION_ERR)

which is admittedly kind of a degenerate case, but it's mostly defensive.

[Joshua Bell]

Comment on attachment 143595 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143595&action=review

> LayoutTests/storage/indexeddb/constants-expected.txt:8
> +PASS IDBDatabaseException.UnknownError is 1

My understanding of the spec is that IDBDatabaseException goes away completely, and that exceptions are all DOMExceptions thrown by name. Is there any benefit in introducing IDBDatabaseException.UnknownError (new name) if we are only keeping IDBDatabaseException.UNKNOWN_ERR for compatibility?

[Alec Flett]

Comment on attachment 143595 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143595&action=review

>> LayoutTests/storage/indexeddb/constants-expected.txt:8
>> +PASS IDBDatabaseException.UnknownError is 1
> 
> My understanding of the spec is that IDBDatabaseException goes away completely, and that exceptions are all DOMExceptions thrown by name. Is there any benefit in introducing IDBDatabaseException.UnknownError (new name) if we are only keeping IDBDatabaseException.UNKNOWN_ERR for compatibility?

It's there entirely for backwards compatibility - if APIs were previously looking at the exception's name field.

See my previous comment in the bug

[Joshua Bell]

(In reply to comment #5)
> It's there entirely for backwards compatibility - if APIs were previously looking at the exception's name field.
> 
> See my previous comment in the bug

Ah, yes, sorry - the "degenerate case". For me, that falls below the threshold for compat support. IMHO we should break those now than add extra surface area to the API to support them.

[Alec Flett]

sounds fine to me..new patch comin' up

[Alec Flett]

Created attachment 143846 [details]
Patch

[Early Warning System Bot]

Comment on attachment 143846 [details]
Patch

Attachment 143846 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12807166

[Alec Flett]

Created attachment 143852 [details]
Patch

[Alec Flett]

ok jsbell@ ready for review again - removed the "new legacy" constants...

[Joshua Bell]

Comment on attachment 143852 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143852&action=review

> Source/WebCore/ChangeLog:11
> +        The IDB spec has evolved to MixedCase, string-based style names

I'd explicitly mention "DOM4-style DOMExceptions" here as a keyword for people to look for.

> Source/WebCore/Modules/indexeddb/IDBDatabaseException.cpp:38
>      const char* const name;

MAYBE:  in DOM4 parlance this is now |type| (per http://www.w3.org/TR/dom/#exception-domexception) - rename? (probably save that for later)

> Source/WebCore/Modules/indexeddb/IDBDatabaseException.cpp:40
> +    const ExceptionCode domCode;

Suggest just |code| instead of |domCode| which matches http://www.w3.org/TR/dom/#exception-domexception - or maybe |legacyCode|

> Source/WebCore/Modules/indexeddb/IDBDatabaseException.cpp:42
> +    { "UnknownError", "An unknown error occurred within Indexed Database.", 0 },

Add a comment that these names/descriptions are from the spec and are IDB specific

> Source/WebCore/Modules/indexeddb/IDBDatabaseException.cpp:51
> +    { "NotFoundError", "An operation failed because the requested database object could not be found.", NOT_FOUND_ERR },

Add  a comment that these are standard DOM4 exception names, but have IDB-specific descriptions

> Source/WebCore/Modules/indexeddb/IDBDatabaseException.h:48
>          NON_TRANSIENT_ERR,

Add a FIXME that NON_TRANSIENT_ERR will be removed (no longer in IDB spec)

> Source/WebCore/Modules/indexeddb/IDBDatabaseException.h:51
>          NOT_ALLOWED_ERR,

Add a FIXME that NOT_ALLOWED_ERR will be removed (no longer in IDB spec)

> Source/WebCore/Modules/indexeddb/IDBDatabaseException.idl:45
> +        const unsigned short CONSTRAINT_ERR = 3;

This renumbering may break code that has hardcoded the number (e.g. via Closure compiler). Should we keep the legacy numbers intact until we actually remove IDBDatabaseException? Would require dummy entries in the C++ code.

[Alec Flett]

Created attachment 143922 [details]
Patch

[Alec Flett]

jsbell@ - ping - hopefully this is just a rubber stamp at this point, comments have been addressed.

(I have another patch coming up which is dependent on this and has more extensive tests too)

[Joshua Bell]

Comment on attachment 143922 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143922&action=review

lgtm with nit

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:216
> +        // FIXME: IDBDatabaseError doesn't yet do proper translation of IDB-specifc uses of error codes, this should be IDB_ABORT_ERR

Typo: "specifc" -> "specific"

[Alec Flett]

Created attachment 144147 [details]
Patch

[Alec Flett]

Created attachment 144582 [details]
Patch

[Alec Flett]

tony@ - r? cq?

[Tony Chang]

Comment on attachment 144582 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144582&action=review

> Source/WebCore/Modules/indexeddb/IDBDatabaseException.cpp:45
> +    { "NonTransientError", "NonTransientError", 0 }, // FIXME: No longer in the spec
> +    { 0, 0, 0 }, // FIXME: Previous/legacy value NOT_FOUND_ERR.

Keeping parallel arrays seems fragile.  Are there plans to move away from this?  E.g., maybe we could use a map or a static sized array where we assign each entry based on the enum values.

[Alec Flett]

Totally agree on parallel arrays, it's very fragile - this happens to be getting a lot of attention right now (read: 2-3 more patches this week against it) so it will stay in sync but long term we'll be moving away from that to a map...

[WebKit Review Bot]

Comment on attachment 144582 [details]
Patch

Clearing flags on attachment: 144582

Committed r118835: <http://trac.webkit.org/changeset/118835>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.