IndexedDB: Make DOMException messages more useful.

IndexedDB: Make DOMException messages more useful.

IndexedDB throws a handful of DOMException types for a wide variety of
reasons; IDBObjectStore.put() has 6 cases that throw DataError.

Show web developers a little love by giving specific messages for
each error cases. In most cases the error text is inspired by the
IndexedDB specification itself.

BUG=95723
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154140

[jsbell, 2013-07-12 17:33:45]

I put all of the re-used messages in IDBDatabase. I could make them more scoped
(e.g. some are only re-used within a file); I don't have a strong preference.

Note that this didn't require rebaselines for any test. I guess that's
consistent with the Web in general which should not depend on specific message
text.

[arv (Not doing code reviews), 2013-07-12 17:43:08]

LGTM with nits

https://codereview.chromium.org/18580013/diff/2001/Source/modules/indexeddb/I...
File Source/modules/indexeddb/IDBCursor.cpp (right):

https://codereview.chromium.org/18580013/diff/2001/Source/modules/indexeddb/I...
Source/modules/indexeddb/IDBCursor.cpp:130:
es.throwDOMException(InvalidStateError, IDBDatabase::notGotValueMessage);
maybe didntGetValueErrorMessage or noValueErrorMessage?

https://codereview.chromium.org/18580013/diff/2001/Source/modules/indexeddb/I...
Source/modules/indexeddb/IDBCursor.cpp:134:
es.throwDOMException(InvalidStateError, IDBDatabase::isKeyCursorMessage);
Can we name these *ErrorMessage?

https://codereview.chromium.org/18580013/diff/2001/Source/modules/indexeddb/I...
File Source/modules/indexeddb/IDBDatabase.cpp (right):

https://codereview.chromium.org/18580013/diff/2001/Source/modules/indexeddb/I...
Source/modules/indexeddb/IDBDatabase.cpp:54: const char
IDBDatabase::noSuchIndexMessage[] = "The specified index was not be found.";
Bad grammar of message

https://codereview.chromium.org/18580013/diff/2001/Source/modules/indexeddb/I...
Source/modules/indexeddb/IDBDatabase.cpp:213:
es.throwDOMException(InvalidAccessError, "The autoIncrement option was set but
the keyPath option was not a non-empty string.");
Double negation. Maybe?

but the keyPath option was missing or an empty string

[jsbell, 2013-07-12 17:53:13]

https://codereview.chromium.org/18580013/diff/2001/Source/modules/indexeddb/I...
File Source/modules/indexeddb/IDBCursor.cpp (right):

https://codereview.chromium.org/18580013/diff/2001/Source/modules/indexeddb/I...
Source/modules/indexeddb/IDBCursor.cpp:130:
es.throwDOMException(InvalidStateError, IDBDatabase::notGotValueMessage);
On 2013/07/12 17:43:08, arv wrote:
> maybe didntGetValueErrorMessage or noValueErrorMessage?

Yeah, that was contorted. I went with "noValueErrorMessage".

https://codereview.chromium.org/18580013/diff/2001/Source/modules/indexeddb/I...
Source/modules/indexeddb/IDBCursor.cpp:134:
es.throwDOMException(InvalidStateError, IDBDatabase::isKeyCursorMessage);
On 2013/07/12 17:43:08, arv wrote:
> Can we name these *ErrorMessage?

Done (while you were typing this, actually!)

https://codereview.chromium.org/18580013/diff/2001/Source/modules/indexeddb/I...
File Source/modules/indexeddb/IDBDatabase.cpp (right):

https://codereview.chromium.org/18580013/diff/2001/Source/modules/indexeddb/I...
Source/modules/indexeddb/IDBDatabase.cpp:54: const char
IDBDatabase::noSuchIndexMessage[] = "The specified index was not be found.";
On 2013/07/12 17:43:08, arv wrote:
> Bad grammar of message

Done.

https://codereview.chromium.org/18580013/diff/2001/Source/modules/indexeddb/I...
Source/modules/indexeddb/IDBDatabase.cpp:213:
es.throwDOMException(InvalidAccessError, "The autoIncrement option was set but
the keyPath option was not a non-empty string.");
On 2013/07/12 17:43:08, arv wrote:
> Double negation. Maybe?
> 
> but the keyPath option was missing or an empty string

Went with "but the keyPath option was empty or an array."

[dgrogan, 2013-07-12 17:58:17]

lgtm

Thanks for doing this. I look forward to seeing these show up on stackoverflow.

https://codereview.chromium.org/18580013/diff/2003/Source/modules/indexeddb/I...
File Source/modules/indexeddb/IDBDatabase.cpp (right):

https://codereview.chromium.org/18580013/diff/2003/Source/modules/indexeddb/I...
Source/modules/indexeddb/IDBDatabase.cpp:208: es.throwDOMException(SyntaxError,
"The keyPath option is not a valid key path.");
What do you think of splitting this up and including the invalid keypath in the
error message? Though I see that throwDOMException only takes a const char*. Why
is that?

https://codereview.chromium.org/18580013/diff/2003/Source/modules/indexeddb/I...
Source/modules/indexeddb/IDBDatabase.cpp:242:
es.throwDOMException(TransactionInactiveError);
Was this skipped intentionally?

https://codereview.chromium.org/18580013/diff/2003/Source/modules/indexeddb/I...
File Source/modules/indexeddb/IDBIndex.cpp (right):

https://codereview.chromium.org/18580013/diff/2003/Source/modules/indexeddb/I...
Source/modules/indexeddb/IDBIndex.cpp:71:
es.throwDOMException(TransactionInactiveError);
I see, that last one probably was skipped intentionally. I suppose there's not
much that a message could add. Which is unfortunate, because I feel like this is
one of the things that will confuse developers when they try to do something
like open a cursor on a transaction from a setTimeout function.

So, if you do think of something helpful, the developer in me would be grateful.
But no worries if you can't, as this CL as-is is a big improvement over the
current state.

[arv (Not doing code reviews), 2013-07-12 18:07:29]

https://codereview.chromium.org/18580013/diff/2003/Source/modules/indexeddb/I...
File Source/modules/indexeddb/IDBDatabase.cpp (right):

https://codereview.chromium.org/18580013/diff/2003/Source/modules/indexeddb/I...
Source/modules/indexeddb/IDBDatabase.cpp:208: es.throwDOMException(SyntaxError,
"The keyPath option is not a valid key path.");
On 2013/07/12 17:58:17, dgrogan wrote:
> What do you think of splitting this up and including the invalid keypath in
the
> error message? Though I see that throwDOMException only takes a const char*.
Why
> is that?

One step at a time. You can use String.format or we can extend ExceptionState to
support other versions.

https://codereview.chromium.org/18580013/diff/2003/Source/modules/indexeddb/I...
File Source/modules/indexeddb/IDBIndex.cpp (right):

https://codereview.chromium.org/18580013/diff/2003/Source/modules/indexeddb/I...
Source/modules/indexeddb/IDBIndex.cpp:71:
es.throwDOMException(TransactionInactiveError);
On 2013/07/12 17:58:17, dgrogan wrote:
> I see, that last one probably was skipped intentionally. I suppose there's not
> much that a message could add. Which is unfortunate, because I feel like this
is
> one of the things that will confuse developers when they try to do something
> like open a cursor on a transaction from a setTimeout function.
> 
> So, if you do think of something helpful, the developer in me would be
grateful.
> But no worries if you can't, as this CL as-is is a big improvement over the
> current state.

It falls back on the default message as defined in DOMException.cpp

[arv (Not doing code reviews), 2013-07-12 18:19:18]

Maybe we should change the test we have to also assert the exception message?

[jsbell, 2013-07-12 18:22:19]

I'll re-do the TransactionInactiveError messages but leave including paths/names
for a future CL. (And keep the bug open.)

https://codereview.chromium.org/18580013/diff/2003/Source/modules/indexeddb/I...
File Source/modules/indexeddb/IDBDatabase.cpp (right):

https://codereview.chromium.org/18580013/diff/2003/Source/modules/indexeddb/I...
Source/modules/indexeddb/IDBDatabase.cpp:208: es.throwDOMException(SyntaxError,
"The keyPath option is not a valid key path.");
On 2013/07/12 18:07:29, arv wrote:
> On 2013/07/12 17:58:17, dgrogan wrote:
> > What do you think of splitting this up and including the invalid keypath in
> the
> > error message? Though I see that throwDOMException only takes a const char*.
> Why
> > is that?
> 
> One step at a time. You can use String.format or we can extend ExceptionState
to
> support other versions.

Yeah, we could go further and include the details (the key path, the not
found/duplicate name, etc). In the case of key path it's slightly tricky as we'd
potentially need to serialize the array.

https://codereview.chromium.org/18580013/diff/2003/Source/modules/indexeddb/I...
Source/modules/indexeddb/IDBDatabase.cpp:242:
es.throwDOMException(TransactionInactiveError);
On 2013/07/12 17:58:17, dgrogan wrote:
> Was this skipped intentionally?

I was thinking the generic message was adequate, but it would be better to
remove the "or which is finished." from it.

[jsbell, 2013-07-12 18:25:51]

On 2013/07/12 18:19:18, arv wrote:
> Maybe we should change the test we have to also assert the exception message?

I'll do a separate CL that logs the message from evalAndExpectException. That
will avoid duplicating the message in the code/test, but keep it in the expected
file which will detect regressions and will make maintaining it easier.

[commit-bot: I haz the power, 2013-07-12 18:33:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jsbell@chromium.org/18580013/21001

[commit-bot: I haz the power, 2013-07-12 20:54:27]

Change committed as 154140