IndexedDB: Align codes and names for IDB-specific and DOM-specific errors/exceptions
Created attachment 143595 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
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.
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?
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
(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.
sounds fine to me..new patch comin' up
Created attachment 143846 [details] Patch
Comment on attachment 143846 [details] Patch Attachment 143846 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12807166
Created attachment 143852 [details] Patch
ok jsbell@ ready for review again - removed the "new legacy" constants...
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.
Created attachment 143922 [details] Patch
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)
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"
Created attachment 144147 [details] Patch
Created attachment 144582 [details] Patch
tony@ - r? cq?
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.
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...
Comment on attachment 144582 [details] Patch Clearing flags on attachment: 144582 Committed r118835: <http://trac.webkit.org/changeset/118835>
All reviewed patches have been landed. Closing bug.