Support renaming of IndexedDB indexes and object stores.

Support renaming of IndexedDB indexes and object stores.

Intent to Implement: https://groups.google.com/a/chromium.org/d/msg/blink-dev/UpS11lcUkYg/NBFTJMQPBgAJ

Index Spec: https://w3c.github.io/IndexedDB/#dom-idbindex-name
Object Store Spec: https://w3c.github.io/IndexedDB/#dom-idbobjectstore-name

BUG=457447
Committed: https://crrev.com/333553242742f0e2acfc966602e2d2e9e2bf3008
Cr-Commit-Position: refs/heads/master@{#417803}

[cmumford, 2016-08-26 21:23:51]

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

[cmumford, 2016-08-26 21:23:52]

https://codereview.chromium.org/2276593002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html
(right):

https://codereview.chromium.org/2276593002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:66:
const createNotBooksStore = function(testCase, database) {
You need to call this somewhere too right? Also, checks for undefined names,
etc.?

[pwnall, 2016-08-26 22:23:18]

Thank you for the quick feedback!

https://codereview.chromium.org/2276593002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html
(right):

https://codereview.chromium.org/2276593002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:66:
const createNotBooksStore = function(testCase, database) {
On 2016/08/26 21:23:52, cmumford wrote:
> You need to call this somewhere too right? Also, checks for undefined names,
> etc.?

Done.

I wrote createNotBooksStore to test the rename behavior in the presence of name
clashes. I finished writing all the error test cases (including name clashes and
undefined), and uploaded them in a new version of this CL.

[pwnall, 2016-08-26 22:23:28]

Patchset #1 (id:1) has been deleted

[pwnall, 2016-08-26 22:23:36]

Patchset #1 (id:20001) has been deleted

[pwnall, 2016-08-26 22:23:46]

Patchset #1 (id:40001) has been deleted

[jsbell, 2016-08-26 23:20:49]

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

[jsbell, 2016-08-26 23:20:50]

A few quick notes on test style and one thing I spotted in the code.

https://codereview.chromium.org/2276593002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html
(right):

https://codereview.chromium.org/2276593002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:2:
<title>IndexedDB v2: object store renaming support</title>
nit: I wouldn't bother with the 'v2' here. (I think you copied that from Moz's
test?)

https://codereview.chromium.org/2276593002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:19:
['abort', 'complete', 'error', 'success', 'upgradeneeded']);
This is only used for requests here, so I'd remove the transaction cases
('abort', 'complete')

https://codereview.chromium.org/2276593002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:34:
request.onupgradeneeded = (event) => {
nit: don't need () around single argument (here and below)

https://codereview.chromium.org/2276593002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:41:
request.onerror =
Do you need to reject() here or does that get handled automagically by
unreached_func for a promise_test ?

https://codereview.chromium.org/2276593002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:51: //
Returns a promise that yields an IndexedDB open success event.
In every use of this, only the result is used not the event itself. Maybe
resolve with the db instead?

https://codereview.chromium.org/2276593002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:57:
return migrateDatabase(testCase, 1, setupCallback);
... => { return y; }

can be simplified to:

... => y

(here and below)

https://codereview.chromium.org/2276593002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:179:
assert: 'An object store can be renamed in a new version change transaction',
Does this assert: test property add much? They're used very rarely in
web-platform-tests.

https://codereview.chromium.org/2276593002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp (right):

https://codereview.chromium.org/2276593002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:78:
exceptionState.throwDOMException(InvalidStateError,
IDBDatabase::notVersionChangeTransactionErrorMessage);
FYI, we have a generic storage/indexeddb/exceptions.html test that exercises
various exception cases. Could add cases there if they're not covered by
dedicated tests.

https://codereview.chromium.org/2276593002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:94: if
(m_transaction->db()->containsObjectStore(name)) {
Do we need a special case for `store.name = store.name;` ?

Spec has: If store’s name is equal to name, terminate these steps.

[pwnall, 2016-08-29 19:19:14]

Patchset #4 (id:120001) has been deleted

[pwnall, 2016-08-29 19:24:13]

jsbell: thank you very much for all your feedback! The tests are better thanks
to it, and I'll write better code in the future.

PTAL (only at the tests).

https://codereview.chromium.org/2276593002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html
(right):

https://codereview.chromium.org/2276593002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:2:
<title>IndexedDB v2: object store renaming support</title>
On 2016/08/26 23:20:50, jsbell wrote:
> nit: I wouldn't bother with the 'v2' here. (I think you copied that from Moz's
> test?)

Done.

https://codereview.chromium.org/2276593002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:19:
['abort', 'complete', 'error', 'success', 'upgradeneeded']);
On 2016/08/26 23:20:50, jsbell wrote:
> This is only used for requests here, so I'd remove the transaction cases
> ('abort', 'complete')

Done.

https://codereview.chromium.org/2276593002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:34:
request.onupgradeneeded = (event) => {
On 2016/08/26 23:20:50, jsbell wrote:
> nit: don't need () around single argument (here and below)

Done.

https://codereview.chromium.org/2276593002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:41:
request.onerror =
On 2016/08/26 23:20:50, jsbell wrote:
> Do you need to reject() here or does that get handled automagically by
> unreached_func for a promise_test ?

Done.
You were right, I needed the reject(). Without it, the test timed out when I
threw an error from onupgradeneeded.

https://codereview.chromium.org/2276593002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:51: //
Returns a promise that yields an IndexedDB open success event.
On 2016/08/26 23:20:50, jsbell wrote:
> In every use of this, only the result is used not the event itself. Maybe
> resolve with the db instead?

Done.

https://codereview.chromium.org/2276593002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:57:
return migrateDatabase(testCase, 1, setupCallback);
On 2016/08/26 23:20:50, jsbell wrote:
> ... => { return y; }
> 
> can be simplified to:
> 
> ... => y
> 
> (here and below)

Done.

I did this simplification whenever possible, and it got my "then"s out of
alignment. Is that a reasonable price for gaining an indentation level in the
code that used to be wrapped in braces?

https://codereview.chromium.org/2276593002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:179:
assert: 'An object store can be renamed in a new version change transaction',
On 2016/08/26 23:20:50, jsbell wrote:
> Does this assert: test property add much? They're used very rarely in
> web-platform-tests.

I removed all of them. I was following the guide below blindly :)

http://darobin.github.io/test-harness-tutorial/docs/using-testharness.html#se...

https://codereview.chromium.org/2276593002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp (right):

https://codereview.chromium.org/2276593002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:78:
exceptionState.throwDOMException(InvalidStateError,
IDBDatabase::notVersionChangeTransactionErrorMessage);
On 2016/08/26 23:20:50, jsbell wrote:
> FYI, we have a generic storage/indexeddb/exceptions.html test that exercises
> various exception cases. Could add cases there if they're not covered by
> dedicated tests.

I think the Web Platform layout tests I wrote cover all the branches, but I
haven't covered the exception messages. I think that needs to happen in a
separate test that can't be upstreamed, because AFAIK messages aren't
standardized.

https://codereview.chromium.org/2276593002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:94: if
(m_transaction->db()->containsObjectStore(name)) {
On 2016/08/26 23:20:50, jsbell wrote:
> Do we need a special case for `store.name = store.name;` ?
> 
> Spec has: If store’s name is equal to name, terminate these steps.
> 

Yes, we do :)

The code here doesn't pass the tests yet. Now that I have the tests, I'll use
them to drive the implementation.

[jsbell, 2016-08-29 20:13:49]

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html (right):

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:80: let
result = request.result;
nit: const

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:81:
testCase.step(() => {
testCase.step() should not be necessary in promise-based tests, since an
assertion will throw and the throw will return into a rejection.

It (or testCase.step_func(function ...)) is needed in async tests to wrap
callbacks so exceptions thrown there are caught/associated with the test. But
with properly structured promise tests it's not needed.

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:101: // If
checkIndexContents fails here, its implementation is incorrect.
Ideally, turn comments like this into assertion messages, so they show up on
failure. Maybe pass a string into checkIndexContents?

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:110:
assert_equals(renamedAuthorIndex.name, 'renamed_by_author');
Try to have a message for each assert (we're not consistent about that but
trying to be).

A good form to use is a sentence with 'should', e.g. "The index should have a
new name"

This forces you to think about what the assertion is actually testing, and helps
the reader understand the purpose of the test (e.g. that behavior matches the
spec) and it's more obvious what the failure is when an assertion does fail.

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:125:
assert_equals(authorIndex.name, 'by_author');
sample message: "IDBIndex from earlier transaction should retain previous name"

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:171:
request.onupgradeneeded = (event) => {
nit: Don't need ()

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:185:
transaction.onabort = () => null;
Needed?

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:186:
transaction.onerror = () => null;
Needed?

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:187:
transaction.abort();
Do we expect the index name to synchronously revert at this point? (should be
consistent with newly created indexes disappearing, database version number
reverting, etc)

either way, should assert what is expected here. :)

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:189:
request.onerror = (event) => reject(event.error);
nit: Don't need ()

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:195: 'index
rename not reverted after transaction abort');
Message should describe the *expected* behavior, i.e. 'index rename should be
reverted after transaction abort'

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:199: const
eventWatcher = requestWatcher(testCase, request);
These two lines could be combined, e.g.:

return requestWatcher(testCase, request).wait_for('success');

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:328: },
'IndexedDB index rename to the name of another index throws');
Can you add a test case for swapping names, i.e.:

store.index('by_author').name = 'tmp';
store.index('by_title').name = 'by_author';
store.index('tmp').name = 'by_title';

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:368:
assert_equals(index.name, "() => null");
Other fun cases:

index.name = {toString: () => { throw new Error('no string for you'); }}; //
throws

index.name = ''; // empty string is valid
index.name = '\x00'; // NUL is valid
index.name = '\uDC00\uD800'; // non-UTF-16 is valid (that's a swapped surrogate
pair)

[pwnall, 2016-08-30 23:07:15]

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

[commit-bot: I haz the power, 2016-08-30 23:08:06]

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

[pwnall, 2016-08-30 23:12:16]

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

[commit-bot: I haz the power, 2016-08-30 23:12:39]

Dry run: 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-08-30 23:48:52]

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

[commit-bot: I haz the power, 2016-08-30 23:48:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pwnall, 2016-09-01 22:10:00]

Patchset #7 (id:200001) has been deleted

[pwnall, 2016-09-01 22:10:15]

Patchset #5 (id:160001) has been deleted

[pwnall, 2016-09-01 23:32:54]

Patchset #6 (id:220001) has been deleted

[pwnall, 2016-09-01 23:33:09]

Patchset #5 (id:180001) has been deleted

[pwnall, 2016-09-01 23:34:16]

Thank you for the feedback, jsbell! I hope I've addressed all of it, and I think
you found a Firefox bug :)

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html (right):

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:80: let
result = request.result;
On 2016/08/29 20:13:48, jsbell wrote:
> nit: const

Done.

I went through my other lets as well.

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:81:
testCase.step(() => {
On 2016/08/29 20:13:48, jsbell wrote:
> testCase.step() should not be necessary in promise-based tests, since an
> assertion will throw and the throw will return into a rejection.
> 
> It (or testCase.step_func(function ...)) is needed in async tests to wrap
> callbacks so exceptions thrown there are caught/associated with the test. But
> with properly structured promise tests it's not needed.

Done. Removed.

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:101: // If
checkIndexContents fails here, its implementation is incorrect.
On 2016/08/29 20:13:49, jsbell wrote:
> Ideally, turn comments like this into assertion messages, so they show up on
> failure. Maybe pass a string into checkIndexContents?

Done.

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:110:
assert_equals(renamedAuthorIndex.name, 'renamed_by_author');
On 2016/08/29 20:13:49, jsbell wrote:
> Try to have a message for each assert (we're not consistent about that but
> trying to be).
> 
> A good form to use is a sentence with 'should', e.g. "The index should have a
> new name"
> 
> This forces you to think about what the assertion is actually testing, and
helps
> the reader understand the purpose of the test (e.g. that behavior matches the
> spec) and it's more obvious what the failure is when an assertion does fail.

Done.

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:125:
assert_equals(authorIndex.name, 'by_author');
On 2016/08/29 20:13:49, jsbell wrote:
> sample message: "IDBIndex from earlier transaction should retain previous
name"

Done.

I ended up with a somewhat different message. Does it look reasonable?

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:171:
request.onupgradeneeded = (event) => {
On 2016/08/29 20:13:48, jsbell wrote:
> nit: Don't need ()

Done.

I made a cleaning pass for these.

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:185:
transaction.onabort = () => null;
On 2016/08/29 20:13:48, jsbell wrote:
> Needed?

Done. Removed :)

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:186:
transaction.onerror = () => null;
On 2016/08/29 20:13:49, jsbell wrote:
> Needed?

Done.

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:187:
transaction.abort();
On 2016/08/29 20:13:49, jsbell wrote:
> Do we expect the index name to synchronously revert at this point? (should be
> consistent with newly created indexes disappearing, database version number
> reverting, etc)
> 
> either way, should assert what is expected here. :)

Done.

Added tests.

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:189:
request.onerror = (event) => reject(event.error);
On 2016/08/29 20:13:49, jsbell wrote:
> nit: Don't need ()

Done.

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:195: 'index
rename not reverted after transaction abort');
On 2016/08/29 20:13:49, jsbell wrote:
> Message should describe the *expected* behavior, i.e. 'index rename should be
> reverted after transaction abort'
> 

Done.

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:199: const
eventWatcher = requestWatcher(testCase, request);
On 2016/08/29 20:13:49, jsbell wrote:
> These two lines could be combined, e.g.:
> 
> return requestWatcher(testCase, request).wait_for('success');

Done.
I did this change in the two places where it reduced line count.

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:328: },
'IndexedDB index rename to the name of another index throws');
On 2016/08/29 20:13:49, jsbell wrote:
> Can you add a test case for swapping names, i.e.:
> 
> store.index('by_author').name = 'tmp';
> store.index('by_title').name = 'by_author';
> store.index('tmp').name = 'by_title';
> 

Done.

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:368:
assert_equals(index.name, "() => null");
On 2016/08/29 20:13:48, jsbell wrote:
> Other fun cases:
> 
> index.name = {toString: () => { throw new Error('no string for you'); }}; //
> throws
> 
> index.name = ''; // empty string is valid
> index.name = '\x00'; // NUL is valid
> index.name = '\uDC00\uD800'; // non-UTF-16 is valid (that's a swapped
surrogate
> pair)
> 

Done.

Firefox fails with '\uDC00\uD800'. Should I file a bug?

[jsbell, 2016-09-01 23:47:09]

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html (right):

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:368:
assert_equals(index.name, "() => null");
On 2016/09/01 23:34:15, pwnall wrote:
> Firefox fails with '\uDC00\uD800'. Should I file a bug?

In what way does it fail?

[pwnall, 2016-09-02 00:11:49]

On 2016/09/01 23:47:09, jsbell wrote:
>
https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
> File third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html
(right):
> 
>
https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:368:
> assert_equals(index.name, "() => null");
> On 2016/09/01 23:34:15, pwnall wrote:
> > Firefox fails with '\uDC00\uD800'. Should I file a bug?
> 
> In what way does it fail?

Gah, indexedDB.open fails because the database name includes the test case name,
which has the failed surrogate. :(
This is not relevant in the context of what I'm testing (fix coming), but it's
still a bug :/

[pwnall, 2016-09-02 02:03:52]

Patchset #6 (id:260001) has been deleted

[pwnall, 2016-09-02 08:06:04]

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

[commit-bot: I haz the power, 2016-09-02 08:06:32]

Dry run: 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-02 08:39:03]

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

[commit-bot: I haz the power, 2016-09-02 08:39:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pwnall, 2016-09-02 20:27:50]

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

[commit-bot: I haz the power, 2016-09-02 20:28:36]

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

[pwnall, 2016-09-02 20:28:56]

Patchset #7 (id:300001) has been deleted

[pwnall, 2016-09-02 20:29:03]

Patchset #8 (id:340001) has been deleted

[pwnall, 2016-09-02 20:30:41]

Patchset #7 (id:320001) has been deleted

[pwnall, 2016-09-02 20:31:11]

Patchset #3 (id:100001) has been deleted

[pwnall, 2016-09-02 20:31:38]

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

[commit-bot: I haz the power, 2016-09-02 20:32:43]

Dry run: 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-02 21:00:04]

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

[commit-bot: I haz the power, 2016-09-02 21:00:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pwnall, 2016-09-02 22:55:12]

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

[commit-bot: I haz the power, 2016-09-02 22:55:41]

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-02 22:57:37]

Whoops, I was slow and now I've got comments spanning two patchsets. Sending now
before I get farther behind.

tests L*G*T*M; we probably want to factor out the helper common functions into a
script file in resources/ (and yes, we can upstream things like that)

https://codereview.chromium.org/2276593002/diff/360001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html (right):

https://codereview.chromium.org/2276593002/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:68:
store.put({ title: 'Quarry Memories', author: 'Fred', isbn: 123456 });
Maybe make a global `const SAMPLE_DATA = [ ... ]` that holds these records...

https://codereview.chromium.org/2276593002/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:81: const
request = index.get('Barney');
... and then use the SAMPLE_DATA[0]'s values rather than hardcoding them here?

https://codereview.chromium.org/2276593002/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:97: const
request = index.get('Bedrock Nights');
ditto

https://codereview.chromium.org/2276593002/diff/360001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html
(right):

https://codereview.chromium.org/2276593002/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:265:
'The object store index should pass before any renaming').then(
Maybe rephrase "should pass", e.g. "should have expected contents" (here and
below)

https://codereview.chromium.org/2276593002/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:287:
'any  renaming').then(() => database.close());
Nit: extra space in string

https://codereview.chromium.org/2276593002/diff/380001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html (right):

https://codereview.chromium.org/2276593002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:67:
store.createIndex('by_title', 'title',  { unique: true });
nit: extra space

https://codereview.chromium.org/2276593002/diff/380001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html
(right):

https://codereview.chromium.org/2276593002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:67:
store.createIndex('by_title', 'title',  { unique: true });
nit: extra space

https://codereview.chromium.org/2276593002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:85:
return migrateDatabase(testCase, 2, (database,  transaction) => {
nit: extra space

https://codereview.chromium.org/2276593002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:287:
'any  renaming').then(() => database.close());
nit: extra space

[pwnall, 2016-09-02 23:10:34]

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

[commit-bot: I haz the power, 2016-09-02 23:10:56]

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

[pwnall, 2016-09-02 23:12:16]

Patchset #8 (id:400001) has been deleted

[pwnall, 2016-09-03 00:46:52]

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

[commit-bot: I haz the power, 2016-09-03 00:47:05]

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

[pwnall, 2016-09-03 00:47:06]

Patchset #8 (id:420001) has been deleted

[commit-bot: I haz the power, 2016-09-03 02:47:42]

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

[commit-bot: I haz the power, 2016-09-03 02:47:44]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pwnall, 2016-09-03 05:01:56]

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

[commit-bot: I haz the power, 2016-09-03 05:02:07]

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

[pwnall, 2016-09-03 05:02:19]

Patchset #8 (id:440001) has been deleted

[pwnall, 2016-09-03 06:01:20]

jsbell: Thank you for the quick turnaround on test reviews! What do you think of
reviewing and landing renaming as it is now, and adding proper metadata revert
support on top of it?

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html (right):

https://codereview.chromium.org/2276593002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:368:
assert_equals(index.name, "() => null");
On 2016/09/01 23:47:09, jsbell wrote:
> On 2016/09/01 23:34:15, pwnall wrote:
> > Firefox fails with '\uDC00\uD800'. Should I file a bug?
> 
> In what way does it fail?

Sorry I forgot to answer this question!
https://bugzilla.mozilla.org/show_bug.cgi?id=1300205

https://codereview.chromium.org/2276593002/diff/360001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html (right):

https://codereview.chromium.org/2276593002/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:68:
store.put({ title: 'Quarry Memories', author: 'Fred', isbn: 123456 });
On 2016/09/02 22:57:36, jsbell wrote:
> Maybe make a global `const SAMPLE_DATA = [ ... ]` that holds these records...

Done.

I called it BOOKS_RECORD_DATA. Please let me know if that's not ok.

https://codereview.chromium.org/2276593002/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:81: const
request = index.get('Barney');
On 2016/09/02 22:57:36, jsbell wrote:
> ... and then use the SAMPLE_DATA[0]'s values rather than hardcoding them here?

Done.

https://codereview.chromium.org/2276593002/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:97: const
request = index.get('Bedrock Nights');
On 2016/09/02 22:57:36, jsbell wrote:
> ditto

Done.

https://codereview.chromium.org/2276593002/diff/360001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html
(right):

https://codereview.chromium.org/2276593002/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:265:
'The object store index should pass before any renaming').then(
On 2016/09/02 22:57:37, jsbell wrote:
> Maybe rephrase "should pass", e.g. "should have expected contents" (here and
> below)

Done.

https://codereview.chromium.org/2276593002/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:287:
'any  renaming').then(() => database.close());
On 2016/09/02 22:57:37, jsbell wrote:
> Nit: extra space in string

Done.

https://codereview.chromium.org/2276593002/diff/380001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html (right):

https://codereview.chromium.org/2276593002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:67:
store.createIndex('by_title', 'title',  { unique: true });
On 2016/09/02 22:57:37, jsbell wrote:
> nit: extra space

Done.

Thanks! How do you spot these? :)

https://codereview.chromium.org/2276593002/diff/380001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html
(right):

https://codereview.chromium.org/2276593002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:67:
store.createIndex('by_title', 'title',  { unique: true });
On 2016/09/02 22:57:37, jsbell wrote:
> nit: extra space

Done.

https://codereview.chromium.org/2276593002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:85:
return migrateDatabase(testCase, 2, (database,  transaction) => {
On 2016/09/02 22:57:37, jsbell wrote:
> nit: extra space

Done.

https://codereview.chromium.org/2276593002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:287:
'any  renaming').then(() => database.close());
On 2016/09/02 22:57:37, jsbell wrote:
> nit: extra space

Done.

[commit-bot: I haz the power, 2016-09-03 06:20:30]

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

[commit-bot: I haz the power, 2016-09-03 06:20:31]

Dry run: This issue passed the CQ dry run.

[pwnall, 2016-09-07 00:48:33]

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

[commit-bot: I haz the power, 2016-09-07 00:48:53]

Dry run: 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-07 02:14:24]

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

[commit-bot: I haz the power, 2016-09-07 02:14:26]

Dry run: This issue passed the CQ dry run.

[jsbell, 2016-09-07 17:16:14]

Mostly L*G*T*M, a few nits in the code and tests.

https://codereview.chromium.org/2276593002/diff/480001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/2276593002/diff/480001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.cc:633: // Store creation is done
synchronously, as it may be followed by
Update comment, e.g. "Store metadata changes are made..."

https://codereview.chromium.org/2276593002/diff/480001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.cc:799: // Index creation is done
synchronously since preemptive
Ditto

https://codereview.chromium.org/2276593002/diff/480001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.cc:813: ASCIIToUTF16("Internal
error creating index '") +
Update error message.

https://codereview.chromium.org/2276593002/diff/480001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_database.h (right):

https://codereview.chromium.org/2276593002/diff/480001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.h:228: const base::string16
old_name,
Can be a const ref.

https://codereview.chromium.org/2276593002/diff/480001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.h:246: const base::string16
old_name,
Ditto.

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html (right):

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:45:
'IDBObjectStore.index should throw when queried using the renamed' +
need a space before the closing '

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:98:
'IDBObjectStore.index should throw when queried using the renamed' +
need a space before the closing '

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:232:
'Committing a transaction with a failed rename attempt should not' +
need a space before the closing '

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:379:
'Committing a transaction with a failed rename attempt should not' +
need a space before the closing '

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:440:
'IDBIndex.name should not reflect the rename anymore ' +
spelling: "any more" (here and below)

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html
(right):

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:281:
'Committing a transaction with a failed rename attempt should not' +
need a space before the closing '

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:568:
'IDBDatabase.objectStoreNames should not reflect the creation' +
need a space before the closing '

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed-expected.txt
(right):

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed-expected.txt:1:
CONSOLE ERROR: line 2445: Uncaught Error: assert_equals: IndexedDB object store
renaming should not be web-exposed expected "books" but got "renamed_books"
This logging on an assertion failure indicates a problem with the test, i.e. an
exception occurring outside a test body or step/step_func wrapper

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html
(right):

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html:5:
// Returns an IndexedDB database name likely to be unique to the test case.
Since this test will be temporary (until we "ship" the feature) and is for the
benefit of API reviewers, can you simplify it? i.e. inline everything, etc?

Also, can you add a comment explaining why we're adding this file? (can't have
setter-only be runtime enabled)

And can you add a TODO with a link to a crbug saying "remove when we ship"

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html:30:
request.onupgradeneeded = event => {
This needs to be testCase.step_func(event => { ... })

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp (right):

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:826: void
IDBObjectStore::indexWillBeRenamed(const String& oldName, const String& newName)
It seems like it would be simpler if this took the index id; it can use that to
look up the old name.

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h (right):

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h:114: void
indexWillBeRenamed(const String& oldName, const String& newName);
"will be" makes it sound like this will happen asynchronously. How about just
"indexRenamed" ?

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp (right):

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:194: void
IDBTransaction::objectStoreWillBeRenamed(const String& oldName, const String&
newName)
Here we don't need the id... but it still seems like passing (id, newName) would
be more natural. But that's just me.

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h (right):

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:89: void
objectStoreWillBeRenamed(const String& oldName, const String& newName);
Same comment as on index: "will be" sounds async.

[pwnall, 2016-09-07 21:01:13]

Description was changed from

==========
WiP: Support renaming of IndexedDB indexes and object stores.

BUG=457447
==========

to

==========
Support renaming of IndexedDB indexes and object stores.

BUG=457447
==========

[pwnall, 2016-09-07 22:14:27]

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

[commit-bot: I haz the power, 2016-09-07 22:14:57]

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

[pwnall, 2016-09-07 22:42:39]

Patchset #10 (id:500001) has been deleted

[pwnall, 2016-09-07 22:43:53]

Thank you for the feedback, and for your patience reviewing those mind-numbing
tests! PTAL?

https://codereview.chromium.org/2276593002/diff/480001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/2276593002/diff/480001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.cc:633: // Store creation is done
synchronously, as it may be followed by
On 2016/09/07 17:16:13, jsbell wrote:
> Update comment, e.g. "Store metadata changes are made..."

Done.

https://codereview.chromium.org/2276593002/diff/480001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.cc:799: // Index creation is done
synchronously since preemptive
On 2016/09/07 17:16:13, jsbell wrote:
> Ditto

Done.

https://codereview.chromium.org/2276593002/diff/480001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.cc:813: ASCIIToUTF16("Internal
error creating index '") +
On 2016/09/07 17:16:13, jsbell wrote:
> Update error message.

Done.

https://codereview.chromium.org/2276593002/diff/480001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_database.h (right):

https://codereview.chromium.org/2276593002/diff/480001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.h:228: const base::string16
old_name,
On 2016/09/07 17:16:13, jsbell wrote:
> Can be a const ref.

Done.

Thank you!

https://codereview.chromium.org/2276593002/diff/480001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.h:246: const base::string16
old_name,
On 2016/09/07 17:16:13, jsbell wrote:
> Ditto.

Done.

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html (right):

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:45:
'IDBObjectStore.index should throw when queried using the renamed' +
On 2016/09/07 17:16:14, jsbell wrote:
> need a space before the closing '

Done.

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:98:
'IDBObjectStore.index should throw when queried using the renamed' +
On 2016/09/07 17:16:14, jsbell wrote:
> need a space before the closing '

Done.

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:232:
'Committing a transaction with a failed rename attempt should not' +
On 2016/09/07 17:16:14, jsbell wrote:
> need a space before the closing '

Done.

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:379:
'Committing a transaction with a failed rename attempt should not' +
On 2016/09/07 17:16:14, jsbell wrote:
> need a space before the closing '

Done.

I also used a regexp to find any other instances.

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-index.html:440:
'IDBIndex.name should not reflect the rename anymore ' +
On 2016/09/07 17:16:14, jsbell wrote:
> spelling: "any more" (here and below)

Done.

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html
(right):

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:281:
'Committing a transaction with a failed rename attempt should not' +
On 2016/09/07 17:16:14, jsbell wrote:
> need a space before the closing '

Done.

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/rename-object-store.html:568:
'IDBDatabase.objectStoreNames should not reflect the creation' +
On 2016/09/07 17:16:14, jsbell wrote:
> need a space before the closing '

Done.

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed-expected.txt
(right):

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed-expected.txt:1:
CONSOLE ERROR: line 2445: Uncaught Error: assert_equals: IndexedDB object store
renaming should not be web-exposed expected "books" but got "renamed_books"
On 2016/09/07 17:16:14, jsbell wrote:
> This logging on an assertion failure indicates a problem with the test, i.e.
an
> exception occurring outside a test body or step/step_func wrapper

Done.

The fix you suggested made this go away :D

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html
(right):

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html:5:
// Returns an IndexedDB database name likely to be unique to the test case.
On 2016/09/07 17:16:14, jsbell wrote:
> Since this test will be temporary (until we "ship" the feature) and is for the
> benefit of API reviewers, can you simplify it? i.e. inline everything, etc?
> 
> Also, can you add a comment explaining why we're adding this file? (can't have
> setter-only be runtime enabled)
> 
> And can you add a TODO with a link to a crbug saying "remove when we ship"

Done.

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html:30:
request.onupgradeneeded = event => {
On 2016/09/07 17:16:14, jsbell wrote:
> This needs to be testCase.step_func(event => { ... })

Done.

Gah.... thanks!!

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp (right):

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:826: void
IDBObjectStore::indexWillBeRenamed(const String& oldName, const String& newName)
On 2016/09/07 17:16:14, jsbell wrote:
> It seems like it would be simpler if this took the index id; it can use that
to
> look up the old name.

Done.

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h (right):

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h:114: void
indexWillBeRenamed(const String& oldName, const String& newName);
On 2016/09/07 17:16:14, jsbell wrote:
> "will be" makes it sound like this will happen asynchronously. How about just
> "indexRenamed" ?

Done.

I initially avoided that because the index's name will change after this
callback.

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp (right):

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:194: void
IDBTransaction::objectStoreWillBeRenamed(const String& oldName, const String&
newName)
On 2016/09/07 17:16:14, jsbell wrote:
> Here we don't need the id... but it still seems like passing (id, newName)
would
> be more natural. But that's just me.

objectStoreDeleted() also takes in the store's name. Should I look into changing
both of them?

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h (right):

https://codereview.chromium.org/2276593002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:89: void
objectStoreWillBeRenamed(const String& oldName, const String& newName);
On 2016/09/07 17:16:14, jsbell wrote:
> Same comment as on index: "will be" sounds async.

Done.

[pwnall, 2016-09-07 22:46:46]

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

[commit-bot: I haz the power, 2016-09-07 22:47:12]

Dry run: 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-08 00:22:18]

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

[commit-bot: I haz the power, 2016-09-08 00:22:19]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[pwnall, 2016-09-08 00:50:22]

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

[commit-bot: I haz the power, 2016-09-08 00:50:34]

Dry run: 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-08 02:17:29]

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

[commit-bot: I haz the power, 2016-09-08 02:17:31]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[pwnall, 2016-09-08 03:32:37]

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

[commit-bot: I haz the power, 2016-09-08 03:32:49]

Dry run: 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-08 06:02:41]

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

[commit-bot: I haz the power, 2016-09-08 06:02:42]

Dry run: This issue passed the CQ dry run.

[pwnall, 2016-09-08 06:44:45]

I removed the failing transaction abort tests, because they were timing out on
the bots. I moved them to http://crrev.com/2314933005 -- I expect that I'll be
able to get them to pass in that CL.

[pwnall, 2016-09-08 22:04:09]

pwnall@chromium.org changed reviewers:
+ foolip@chromium.org

[pwnall, 2016-09-08 22:04:11]

foolip@chromium.org: Please review changes in the webexposed expectations and
the new test at
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html

Context: our bindings generator don't allow us to expose a property's setter
(independently from the getter) based on 
Runtime Enabled Features. We work around this with custom code that makes the
setters introduced by this CL no-ops when the relevant feature is disabled. Most
code shouldn't be able to tell the difference, but the modified properties'
descriptors do reflect the addition of a setter.

[pwnall, 2016-09-08 22:11:47]

pwnall@chromium.org changed reviewers:
+ jochen@chromium.org

[pwnall, 2016-09-08 22:11:48]

jochen@chromium.org: Please review changes in
content/common/indexed_db/indexed_db_messages.h and
content/child/indexed_db/webidbdatabase_impl.{h,cc}

[pwnall, 2016-09-08 22:11:48]

pwnall@chromium.org changed reviewers:
+ jochen@chromium.org

[pwnall, 2016-09-08 22:11:50]

jochen@chromium.org: Please review changes in
content/common/indexed_db/indexed_db_messages.h and
content/child/indexed_db/webidbdatabase_impl.{h,cc}

[pwnall, 2016-09-08 22:51:40]

pwnall@chromium.org changed reviewers:
+ kenrb@chromium.org

[pwnall, 2016-09-08 22:51:41]

kenrb@chromium.org: Please review
content/common/indexed_db/indexed_db_messages.h for IPC security.

Context: The new messages for renaming IndexedDB object stores and indexes have
similar checks to the messages for deleting stores / indexes, and I think they
have the same security implications. A renderer that is able to delete an
{object store, index} should also be able to rename it.

[jsbell, 2016-09-08 22:55:14]

On 2016/09/08 22:11:48, pwnall wrote:
> mailto:jochen@chromium.org: Please review changes in
> content/common/indexed_db/indexed_db_messages.h and
> content/child/indexed_db/webidbdatabase_impl.{h,cc}

I can review the content/child/indexed_db changes. We only need a security
reviewer for indexed_db_messages.h

[jsbell, 2016-09-08 22:56:18]

On 2016/09/08 22:04:11, pwnall wrote:
> mailto:foolip@chromium.org: Please review changes in the webexposed
expectations and
> the new test at
>
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html
> 
> Context: our bindings generator don't allow us to expose a property's setter
> (independently from the getter) based on 
> Runtime Enabled Features. We work around this with custom code that makes the
> setters introduced by this CL no-ops when the relevant feature is disabled.
Most
> code shouldn't be able to tell the difference, but the modified properties'
> descriptors do reflect the addition of a setter.

Please add a link to the spec and Intent to Implement thread in the CL
description. See https://codereview.chromium.org/1407883002 for an example.

[pwnall, 2016-09-08 23:01:24]

Description was changed from

==========
Support renaming of IndexedDB indexes and object stores.

BUG=457447
==========

to

==========
Support renaming of IndexedDB indexes and object stores.

Intent to Implement:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/UpS11lcUkYg/NBFTJMQP...

Index Spec: https://w3c.github.io/IndexedDB/#dom-idbindex-name
Object Store Spec: https://w3c.github.io/IndexedDB/#dom-idbobjectstore-name


BUG=457447
==========

[pwnall, 2016-09-08 23:01:45]

Description was changed from

==========
Support renaming of IndexedDB indexes and object stores.

Intent to Implement:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/UpS11lcUkYg/NBFTJMQP...

Index Spec: https://w3c.github.io/IndexedDB/#dom-idbindex-name
Object Store Spec: https://w3c.github.io/IndexedDB/#dom-idbobjectstore-name


BUG=457447
==========

to

==========
Support renaming of IndexedDB indexes and object stores.

Intent to Implement:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/UpS11lcUkYg/NBFTJMQP...

Index Spec: https://w3c.github.io/IndexedDB/#dom-idbindex-name
Object Store Spec: https://w3c.github.io/IndexedDB/#dom-idbobjectstore-name

BUG=457447
==========

[pwnall, 2016-09-08 23:04:04]

pwnall@chromium.org changed reviewers:
- jochen@chromium.org

[pwnall, 2016-09-08 23:04:05]

Sparing jochen of reviewing this CL, per jsbell's comment.

[jsbell, 2016-09-08 23:16:21]

content/{browser,child}/indexed_db
third_party/WebKit/LayoutTests/imported/wpt/IndexedDB
third_party/WebKit/LayoutTests/storage/indexeddb
third_party/WebKit/Source/modules/indexeddb
third_party/WebKit/public/platform/modules/indexeddb
... all LGTM

https://codereview.chromium.org/2276593002/diff/560001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/storage/indexeddb/resources/rename-common.js
(right):

https://codereview.chromium.org/2276593002/diff/560001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/rename-common.js:1: 
nit: remove leading blank line

[pwnall, 2016-09-09 01:29:12]

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

[commit-bot: I haz the power, 2016-09-09 01:29:26]

Dry run: 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-09 03:13:17]

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

[commit-bot: I haz the power, 2016-09-09 03:13:19]

Dry run: This issue passed the CQ dry run.

[foolip, 2016-09-09 08:12:17]

https://codereview.chromium.org/2276593002/diff/600001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html
(right):

https://codereview.chromium.org/2276593002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html:13:
async_test(testCase => {
FWIW, in upstream web-platform-tests, this seems to be called either test or
just t, or alternatively one can use this throughout, as the method is invoked
with the test as the this value. Not sure if that works with arrow functions
though.

https://codereview.chromium.org/2276593002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html:14:
const dbName = 'db' + self.location.pathname + '-' + testCase.name;
Looks like the web platform is missing an mktemp for databases :)

https://codereview.chromium.org/2276593002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html:31:
request.onsuccess = event => {
testCase.step_func, or even step_func_done if you like that style.

https://codereview.chromium.org/2276593002/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBIndex.cpp (right):

https://codereview.chromium.org/2276593002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBIndex.cpp:70: if
(!RuntimeEnabledFeatures::indexedDBExperimentalEnabled())
With 'use strict', trying to set a readonly property should actually throw
TypeError. I don't think trying to emulate that here makes sense, so the choice
is between silent return or throwing. Whichever you pick, can you add another a
test that fails on stable but will start passing once this is enabled?

https://codereview.chromium.org/2276593002/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp (right):

https://codereview.chromium.org/2276593002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:77: return;
Ditto.

[foolip, 2016-09-09 08:13:34]

On 2016/09/08 22:04:11, pwnall wrote:
> mailto:foolip@chromium.org: Please review changes in the webexposed
expectations and
> the new test at
>
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html
> 
> Context: our bindings generator don't allow us to expose a property's setter
> (independently from the getter) based on 
> Runtime Enabled Features. We work around this with custom code that makes the
> setters introduced by this CL no-ops when the relevant feature is disabled.
Most
> code shouldn't be able to tell the difference, but the modified properties'
> descriptors do reflect the addition of a setter.

Done, those bits LGTM.

Note that you might actually be able to do what you want if you have two partial
interfaces with the name attribute, where only one of the interfaces is enabled
at the same time. Probably overkill, but try it if you like.

[jsbell, 2016-09-09 18:17:22]

https://codereview.chromium.org/2276593002/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBIndex.cpp (right):

https://codereview.chromium.org/2276593002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBIndex.cpp:70: if
(!RuntimeEnabledFeatures::indexedDBExperimentalEnabled())
On 2016/09/09 08:12:17, foolip wrote:
> can you add another a
> test that fails on stable but will start passing once this is enabled?

Did you want something different than what's captured in
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html
? (that has the reversed pass/fail semantics than what you suggested)

[foolip, 2016-09-09 20:04:34]

https://codereview.chromium.org/2276593002/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBIndex.cpp (right):

https://codereview.chromium.org/2276593002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBIndex.cpp:70: if
(!RuntimeEnabledFeatures::indexedDBExperimentalEnabled())
On 2016/09/09 18:17:22, jsbell wrote:
> On 2016/09/09 08:12:17, foolip wrote:
> > can you add another a
> > test that fails on stable but will start passing once this is enabled?
> 
> Did you want something different than what's captured in
>
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html
> ? (that has the reversed pass/fail semantics than what you suggested)

I was thinking about something like this:

assert_throws(new TypeError, function() {
  'use strict';
  store.name = 'foo';
});

That would pass before this change and once this workaround is removed again.
But it's a bit pedantic, OK with me to not test it.

[pwnall, 2016-09-09 20:50:35]

Thanks for the quick feedback, foolip and jsbell!

foolip: PTAL?

https://codereview.chromium.org/2276593002/diff/560001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/storage/indexeddb/resources/rename-common.js
(right):

https://codereview.chromium.org/2276593002/diff/560001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/rename-common.js:1: 
On 2016/09/08 23:16:21, jsbell wrote:
> nit: remove leading blank line

Done.

https://codereview.chromium.org/2276593002/diff/600001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html
(right):

https://codereview.chromium.org/2276593002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html:13:
async_test(testCase => {
On 2016/09/09 08:12:17, foolip wrote:
> FWIW, in upstream web-platform-tests, this seems to be called either test or
> just t, or alternatively one can use this throughout, as the method is invoked
> with the test as the this value. Not sure if that works with arrow functions
> though.

Done.

I used t, though it doesn't mesh well with my short memory :)

In arrow functions, this has the same value as when the function is defined. I
the fancy speak for this is -- "this" is lexically bound.

https://codereview.chromium.org/2276593002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html:14:
const dbName = 'db' + self.location.pathname + '-' + testCase.name;
On 2016/09/09 08:12:17, foolip wrote:
> Looks like the web platform is missing an mktemp for databases :)

What it really needs is a makeAllMyTestsPass() :D

Seriously though, this approach has the benefit that the database name is
deterministic and based on the test case. When debugging, this makes it easy to
find the database for the test in the dev tools / inspector, and look at its
contents.

https://codereview.chromium.org/2276593002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html:31:
request.onsuccess = event => {
On 2016/09/09 08:12:17, foolip wrote:
> testCase.step_func, or even step_func_done if you like that style.

Done.

https://codereview.chromium.org/2276593002/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBIndex.cpp (right):

https://codereview.chromium.org/2276593002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBIndex.cpp:70: if
(!RuntimeEnabledFeatures::indexedDBExperimentalEnabled())
On 2016/09/09 20:04:34, foolip wrote:
> On 2016/09/09 18:17:22, jsbell wrote:
> > On 2016/09/09 08:12:17, foolip wrote:
> > > can you add another a
> > > test that fails on stable but will start passing once this is enabled?
> > 
> > Did you want something different than what's captured in
> >
>
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html
> > ? (that has the reversed pass/fail semantics than what you suggested)
> 
> I was thinking about something like this:
> 
> assert_throws(new TypeError, function() {
>   'use strict';
>   store.name = 'foo';
> });
> 
> That would pass before this change and once this workaround is removed again.
> But it's a bit pedantic, OK with me to not test it.

I wrote the test and jsbell helped me figure out what it'd take for it to pass.
It seems like I can only determine if the setter was called from strict mode is
to write a custom binding. I think that's overkill, because we plan to ship this
pretty soon, together with all the other IndexedDB v2 changes.

WDYT? Are you ok with the strict mode behavior being incorrect until we ship?

[pwnall, 2016-09-09 20:51:40]

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

[commit-bot: I haz the power, 2016-09-09 20:52:28]

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

[kenrb, 2016-09-09 21:21:40]

ipc lgtm

[pwnall, 2016-09-09 22:24:09]

https://codereview.chromium.org/2276593002/diff/640001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html
(right):

https://codereview.chromium.org/2276593002/diff/640001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html:32:
// In strict mode, attempting to set a read-only property should throw
foolip: I added a test that covers our strict mode behavior, together with an
explanation of why we went that route. Is this acceptable?

[pwnall, 2016-09-09 22:24:33]

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

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

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

[foolip, 2016-09-09 22:31:17]

lgtm

https://codereview.chromium.org/2276593002/diff/600001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html
(right):

https://codereview.chromium.org/2276593002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html:13:
async_test(testCase => {
On 2016/09/09 20:50:34, pwnall wrote:
> On 2016/09/09 08:12:17, foolip wrote:
> > FWIW, in upstream web-platform-tests, this seems to be called either test or
> > just t, or alternatively one can use this throughout, as the method is
invoked
> > with the test as the this value. Not sure if that works with arrow functions
> > though.
> 
> Done.
> 
> I used t, though it doesn't mesh well with my short memory :)
> 
> In arrow functions, this has the same value as when the function is defined. I
> the fancy speak for this is -- "this" is lexically bound.

Right, so it wouldn't work to use only arrow functions. Anyway, doesn't matter.

https://codereview.chromium.org/2276593002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html:14:
const dbName = 'db' + self.location.pathname + '-' + testCase.name;
On 2016/09/09 20:50:34, pwnall wrote:
> On 2016/09/09 08:12:17, foolip wrote:
> > Looks like the web platform is missing an mktemp for databases :)
> 
> What it really needs is a makeAllMyTestsPass() :D
> 
> Seriously though, this approach has the benefit that the database name is
> deterministic and based on the test case. When debugging, this makes it easy
to
> find the database for the test in the dev tools / inspector, and look at its
> contents.

Acknowledged.

https://codereview.chromium.org/2276593002/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBIndex.cpp (right):

https://codereview.chromium.org/2276593002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBIndex.cpp:70: if
(!RuntimeEnabledFeatures::indexedDBExperimentalEnabled())
On 2016/09/09 20:50:35, pwnall wrote:
> On 2016/09/09 20:04:34, foolip wrote:
> > On 2016/09/09 18:17:22, jsbell wrote:
> > > On 2016/09/09 08:12:17, foolip wrote:
> > > > can you add another a
> > > > test that fails on stable but will start passing once this is enabled?
> > > 
> > > Did you want something different than what's captured in
> > >
> >
>
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html
> > > ? (that has the reversed pass/fail semantics than what you suggested)
> > 
> > I was thinking about something like this:
> > 
> > assert_throws(new TypeError, function() {
> >   'use strict';
> >   store.name = 'foo';
> > });
> > 
> > That would pass before this change and once this workaround is removed
again.
> > But it's a bit pedantic, OK with me to not test it.
> 
> I wrote the test and jsbell helped me figure out what it'd take for it to
pass.
> It seems like I can only determine if the setter was called from strict mode
is
> to write a custom binding. I think that's overkill, because we plan to ship
this
> pretty soon, together with all the other IndexedDB v2 changes.
> 
> WDYT? Are you ok with the strict mode behavior being incorrect until we ship?

Yeah, that seems OK.

https://codereview.chromium.org/2276593002/diff/640001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html
(right):

https://codereview.chromium.org/2276593002/diff/640001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webexposed/indexeddb-renames-should-not-be-exposed.html:32:
// In strict mode, attempting to set a read-only property should throw
On 2016/09/09 22:24:09, pwnall wrote:
> foolip: I added a test that covers our strict mode behavior, together with an
> explanation of why we went that route. Is this acceptable?

Yep, looks good!

[commit-bot: I haz the power, 2016-09-10 02:20:29]

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

[commit-bot: I haz the power, 2016-09-10 02:20:30]

Dry run: This issue passed the CQ dry run.

[pwnall, 2016-09-10 02:21:48]

The CQ bit was checked by pwnall@chromium.org

[pwnall, 2016-09-10 02:21:48]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org,
kenrb@chromium.org
Link to the patchset: https://codereview.chromium.org/2276593002/#ps640001
(title: "Added test coverage for the (slightly incorrect) behavior in strict
mode when our flag is not enabl…")

[commit-bot: I haz the power, 2016-09-10 02:22:04]

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-10 02:26:59]

Description was changed from

==========
Support renaming of IndexedDB indexes and object stores.

Intent to Implement:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/UpS11lcUkYg/NBFTJMQP...

Index Spec: https://w3c.github.io/IndexedDB/#dom-idbindex-name
Object Store Spec: https://w3c.github.io/IndexedDB/#dom-idbobjectstore-name

BUG=457447
==========

to

==========
Support renaming of IndexedDB indexes and object stores.

Intent to Implement:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/UpS11lcUkYg/NBFTJMQP...

Index Spec: https://w3c.github.io/IndexedDB/#dom-idbindex-name
Object Store Spec: https://w3c.github.io/IndexedDB/#dom-idbobjectstore-name

BUG=457447
==========

[commit-bot: I haz the power, 2016-09-10 02:27:01]

Committed patchset #16 (id:640001)

[commit-bot: I haz the power, 2016-09-10 02:29:44]

Description was changed from

==========
Support renaming of IndexedDB indexes and object stores.

Intent to Implement:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/UpS11lcUkYg/NBFTJMQP...

Index Spec: https://w3c.github.io/IndexedDB/#dom-idbindex-name
Object Store Spec: https://w3c.github.io/IndexedDB/#dom-idbobjectstore-name

BUG=457447
==========

to

==========
Support renaming of IndexedDB indexes and object stores.

Intent to Implement:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/UpS11lcUkYg/NBFTJMQP...

Index Spec: https://w3c.github.io/IndexedDB/#dom-idbindex-name
Object Store Spec: https://w3c.github.io/IndexedDB/#dom-idbobjectstore-name

BUG=457447

Committed: https://crrev.com/333553242742f0e2acfc966602e2d2e9e2bf3008
Cr-Commit-Position: refs/heads/master@{#417803}
==========

[commit-bot: I haz the power, 2016-09-10 02:29:46]

Patchset 16 (id:??) landed as
https://crrev.com/333553242742f0e2acfc966602e2d2e9e2bf3008
Cr-Commit-Position: refs/heads/master@{#417803}