IndexedDB: Align exception priorities with Firefox

IndexedDB: Align exception priorities with Firefox

In cases where methods are called with multiple failure conditions,
there's a choice of what exception to throw. For example, on a cursor
update() call, if the transaction is inactive *and* the transaction is
read-only *and* the cursor is advancing *and* the cursor's store has
been deleted *and* the new value is invalid, one of several types of
exceptions would be thrown. This CL aligns Blink with Gecko in the
handful of cases where they differed so the same type is thrown.

No meaningful web-facing change - users couldn't compatibly expect a
particular exception type in these cases anyway. (Testing also
revealed a bug in Firefox, reported to their IDB team.)

Committed: https://crrev.com/dce205414aa1bde4125c3129fef24a8dc8ad32cc
Cr-Commit-Position: refs/heads/master@{#416080}

[jsbell, 2016-04-19 21:50:08]

jsbell@chromium.org changed reviewers:
+ cmumford@chromium.org

[jsbell, 2016-04-19 21:50:09]

cmumford@ - please take a look?

(very low priority)

[jsbell, 2016-04-25 21:03:51]

Hold off on reviewing; I've pinged the Gecko folks to see if they want to do any
ordering tweaks before we lock this all in.

[cmumford, 2016-04-25 22:12:30]

On 2016/04/25 21:03:51, jsbell wrote:
> Hold off on reviewing; I've pinged the Gecko folks to see if they want to do
any
> ordering tweaks before we lock this all in.

ack.

[jsbell, 2016-08-31 18:08:35]

Rebooting this: Moz hasn't changed anything so let's go ahead and get this in.
Low pri but should be harmless.

[jsbell, 2016-09-01 18:03:33]

Description was changed from

==========
IndexedDB: Align exception priorities with Firefox

In cases where methods are called with multiple failure conditions,
there's a choice of what exception to throw. For example, on a cursor
update() call, if the transaction is inactive *and* the transaction is
read-only *and* the cursor is advancing *and* the cursor's store has
been deleted *and* the new value is invalid, one of several types of
exceptions would be thrown. This CL aligns Blink with Gecko in the
handful of cases where they differed so the same type is thrown.

No meaningful web-facing change - users couldn't compatibly expect a
particular exception type in these cases anyway. (Testing also
revealed a bug in Firefox, reported to their IDB team.)
==========

to

==========
IndexedDB: Align exception priorities with Firefox

In cases where methods are called with multiple failure conditions,
there's a choice of what exception to throw. For example, on a cursor
update() call, if the transaction is inactive *and* the transaction is
read-only *and* the cursor is advancing *and* the cursor's store has
been deleted *and* the new value is invalid, one of several types of
exceptions would be thrown. This CL aligns Blink with Gecko in the
handful of cases where they differed so the same type is thrown.

No meaningful web-facing change - users couldn't compatibly expect a
particular exception type in these cases anyway. (Testing also
revealed a bug in Firefox, reported to their IDB team.)
==========

[jsbell, 2016-09-01 18:03:33]

jsbell@chromium.org changed reviewers:
+ pwnall@chromium.org
- cmumford@chromium.org

[jsbell, 2016-09-01 18:03:45]

pwnall@ - can you review?

[pwnall, 2016-09-01 19:03:52]

The code changes look good, assuming they pass the tests.

Asides from the line comments on the tests, I'm wondering if it makes sense to
have per-function test files, e.g. cursor-delete-exception-ordering.html.

I'm also wondering what's the rationale for picking the pairs of exceptions that
we test for precedence. Testing all pairs would be thorough, but it also seems
like overkill, as an N-step function would require O(N^2) tests.

TBH, I expected to see tests for consecutive tests, which requires N-1 tests for
an N-step function. But I see that in some cases we're closer to the O(N^2)
case, and in some cases we're missing tests. I'd like to know how we choose.

https://codereview.chromium.org/1897253003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/storage/indexeddb/exception-order.html
(right):

https://codereview.chromium.org/1897253003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/exception-order.html:3: <script
src="../../resources/testharness.js"></script>
Would it make sense to point to the spec that you're testing? For example,
<link rel="help" href="https://w3c.github.io/IndexedDB/">

https://codereview.chromium.org/1897253003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/exception-order.html:8:
indexeddb_test(
Can we upstream tests that reference helpers? I'm asking because I expect that
we'll want to upstream this, for Web Platform predictability.

https://codereview.chromium.org/1897253003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/exception-order.html:19: },
'Connection closed check should precede stores check');
Should the comment above be merged into the assertion text?

For example, "InvalidStateError (connection closed) check should precede
InvalidAccessError (stores is empty) check."

https://codereview.chromium.org/1897253003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/exception-order.html:24: 
Would it make sense to also have a test for InvalidStateError (step 3) vs
NotFoundError (step 5)?

https://codereview.chromium.org/1897253003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/exception-order.html:44:
assert_throws('SyntaxError', function() {
I think the spec [1] says exactly the opposite. The ConstraintError check is at
step 4, and the SyntaxError is at step 6. Do we need to change the spec?

[1] https://w3c.github.io/IndexedDB/#dom-idbdatabase-createobjectstore

https://codereview.chromium.org/1897253003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/exception-order.html:49: },
'Keypad validation should precede store name check');
Keypad -> keypath?

https://codereview.chromium.org/1897253003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/exception-order.html:104: 
Would it also make sense to test TransactionInactiveError vs InvalidStateError?

https://codereview.chromium.org/1897253003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/exception-order.html:145: // *
InvalidStateError (cursor already advancing)
The spec [1] lists 3 causes that could end up in InvalidStateError. Is it worth
testing against all 3?

[1] https://w3c.github.io/IndexedDB/#dom-idbcursor-delete

[jsbell, 2016-09-01 20:19:30]

The CQ bit was checked by jsbell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 20:20:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jsbell, 2016-09-01 20:20:46]

On 2016/09/01 19:03:52, pwnall wrote:
> Asides from the line comments on the tests, I'm wondering if it makes sense to
> have per-function test files, e.g. cursor-delete-exception-ordering.html.

Done.
 
> TBH, I expected to see tests for consecutive tests, which requires N-1 tests
for
> an N-step function.

Agreed. It's been a long time since I wrote the CL but I suspect I just added
test cases for where we differed from Firefox and needed a code change, rather
than being comprehensive. We also don't cover all the methods. (These tests are
a bit mind-numbing to write.)

https://codereview.chromium.org/1897253003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/storage/indexeddb/exception-order.html
(right):

https://codereview.chromium.org/1897253003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/exception-order.html:3: <script
src="../../resources/testharness.js"></script>
On 2016/09/01 19:03:52, pwnall wrote:
> Would it make sense to point to the spec that you're testing? For example,
> <link rel="help" href="https://w3c.github.io/IndexedDB/">

Done.

https://codereview.chromium.org/1897253003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/exception-order.html:8:
indexeddb_test(
On 2016/09/01 19:03:52, pwnall wrote:
> Can we upstream tests that reference helpers? I'm asking because I expect that
> we'll want to upstream this, for Web Platform predictability.

Yes. There's a variation of this in web-platform-test/IndexedDB/support.js

https://codereview.chromium.org/1897253003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/exception-order.html:19: },
'Connection closed check should precede stores check');
On 2016/09/01 19:03:52, pwnall wrote:
> Should the comment above be merged into the assertion text?
> 
> For example, "InvalidStateError (connection closed) check should precede
> InvalidAccessError (stores is empty) check."

Done.

https://codereview.chromium.org/1897253003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/exception-order.html:24: 
On 2016/09/01 19:03:52, pwnall wrote:
> Would it make sense to also have a test for InvalidStateError (step 3) vs
> NotFoundError (step 5)?

Done.

https://codereview.chromium.org/1897253003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/exception-order.html:44:
assert_throws('SyntaxError', function() {
On 2016/09/01 19:03:52, pwnall wrote:
> I think the spec [1] says exactly the opposite. The ConstraintError check is
at
> step 4, and the SyntaxError is at step 6. Do we need to change the spec?
> 
> [1] https://w3c.github.io/IndexedDB/#dom-idbdatabase-createobjectstore

Yes. Firefox matches the code here, so I'll just update the spec.

https://github.com/w3c/IndexedDB/issues/88

https://codereview.chromium.org/1897253003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/exception-order.html:49: },
'Keypad validation should precede store name check');
On 2016/09/01 19:03:52, pwnall wrote:
> Keypad -> keypath?

Done.

https://codereview.chromium.org/1897253003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/exception-order.html:104: 
On 2016/09/01 19:03:52, pwnall wrote:
> Would it also make sense to test TransactionInactiveError vs
InvalidStateError?

Done.

https://codereview.chromium.org/1897253003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/exception-order.html:145: // *
InvalidStateError (cursor already advancing)
On 2016/09/01 19:03:52, pwnall wrote:
> The spec [1] lists 3 causes that could end up in InvalidStateError. Is it
worth
> testing against all 3?
> 
> [1] https://w3c.github.io/IndexedDB/#dom-idbcursor-delete

I did 2 of 3 (and ditto for "update"). You can't have both "is read only" and
"source is deleted" (mutually exclusive transaction types)

[pwnall, 2016-09-01 20:32:10]

LGTM.

I agree on the mind-numbingness :D I considered precedence tests for renaming,
and backed out when I saw all the cases involved.

[commit-bot: I haz the power, 2016-09-01 21:41:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 21:41:02]

Dry run: This issue passed the CQ dry run.

[pwnall, 2016-09-01 22:09:27]

The CQ bit was checked by pwnall@chromium.org

[commit-bot: I haz the power, 2016-09-01 22:10:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 22:14:50]

Description was changed from

==========
IndexedDB: Align exception priorities with Firefox

In cases where methods are called with multiple failure conditions,
there's a choice of what exception to throw. For example, on a cursor
update() call, if the transaction is inactive *and* the transaction is
read-only *and* the cursor is advancing *and* the cursor's store has
been deleted *and* the new value is invalid, one of several types of
exceptions would be thrown. This CL aligns Blink with Gecko in the
handful of cases where they differed so the same type is thrown.

No meaningful web-facing change - users couldn't compatibly expect a
particular exception type in these cases anyway. (Testing also
revealed a bug in Firefox, reported to their IDB team.)
==========

to

==========
IndexedDB: Align exception priorities with Firefox

In cases where methods are called with multiple failure conditions,
there's a choice of what exception to throw. For example, on a cursor
update() call, if the transaction is inactive *and* the transaction is
read-only *and* the cursor is advancing *and* the cursor's store has
been deleted *and* the new value is invalid, one of several types of
exceptions would be thrown. This CL aligns Blink with Gecko in the
handful of cases where they differed so the same type is thrown.

No meaningful web-facing change - users couldn't compatibly expect a
particular exception type in these cases anyway. (Testing also
revealed a bug in Firefox, reported to their IDB team.)
==========

[commit-bot: I haz the power, 2016-09-01 22:14:52]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-01 22:16:51]

Description was changed from

==========
IndexedDB: Align exception priorities with Firefox

In cases where methods are called with multiple failure conditions,
there's a choice of what exception to throw. For example, on a cursor
update() call, if the transaction is inactive *and* the transaction is
read-only *and* the cursor is advancing *and* the cursor's store has
been deleted *and* the new value is invalid, one of several types of
exceptions would be thrown. This CL aligns Blink with Gecko in the
handful of cases where they differed so the same type is thrown.

No meaningful web-facing change - users couldn't compatibly expect a
particular exception type in these cases anyway. (Testing also
revealed a bug in Firefox, reported to their IDB team.)
==========

to

==========
IndexedDB: Align exception priorities with Firefox

In cases where methods are called with multiple failure conditions,
there's a choice of what exception to throw. For example, on a cursor
update() call, if the transaction is inactive *and* the transaction is
read-only *and* the cursor is advancing *and* the cursor's store has
been deleted *and* the new value is invalid, one of several types of
exceptions would be thrown. This CL aligns Blink with Gecko in the
handful of cases where they differed so the same type is thrown.

No meaningful web-facing change - users couldn't compatibly expect a
particular exception type in these cases anyway. (Testing also
revealed a bug in Firefox, reported to their IDB team.)

Committed: https://crrev.com/dce205414aa1bde4125c3129fef24a8dc8ad32cc
Cr-Commit-Position: refs/heads/master@{#416080}
==========

[commit-bot: I haz the power, 2016-09-01 22:16:52]

Patchset 2 (id:??) landed as
https://crrev.com/dce205414aa1bde4125c3129fef24a8dc8ad32cc
Cr-Commit-Position: refs/heads/master@{#416080}