Bug 59465 – Port Mozilla's IndexedDB tests: readonly and read/write transactions

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

[Mark Pilgrim (Google)]

Original test: http://mxr.mozilla.org/mozilla2.0/source/dom/indexedDB/test/test_readonly_transactions.html?force=1

This is a port of Mozilla's IndexedDB test that tests both readonly and read/write transaction modes. WebKit passes these tests.

[Mark Pilgrim (Google)]

Created attachment 91120 [details]
patch with changelog and layouttest

[David Grogan]

Comment on attachment 91120 [details]
patch with changelog and layouttest

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

Substance is good, just style stuff

> third_party/WebKit/LayoutTests/storage/indexeddb/mozilla/readonly-transactions.html:47
> +function cleanDatabase()

Make more accurate/descriptive.  You can probably ignore the deleteAllObjectStores calls and instead describe the other stuff they do.

> third_party/WebKit/LayoutTests/storage/indexeddb/mozilla/readonly-transactions.html:51
> +    osName = "test store";

I think the Webkit Way is to abbreviate as little as possible.  rename osName -> objectStoreName

> third_party/WebKit/LayoutTests/storage/indexeddb/mozilla/readwrite-transactions.html:26
> +    indexedDB = evalAndLog("indexedDB = window.indexedDB || window.webkitIndexedDB || window.mozIndexedDB;");

at some point we should probably refactor some or all of this into a function that lives in resources/shared.js

> third_party/WebKit/LayoutTests/storage/indexeddb/mozilla/readwrite-transactions.html:49
> +function cleanDatabase()

same comment as usual about cleanDatabase

> third_party/WebKit/LayoutTests/storage/indexeddb/mozilla/readwrite-transactions.html:63
> +    key1 = evalAndLog("key1 = event.target.result;");

move this to postAdd2, right before key1 is first used

> third_party/WebKit/LayoutTests/storage/indexeddb/mozilla/readwrite-transactions.html:72
> +    key2 = evalAndLog("key2 = event.target.result;");

move this

[Tony Chang]

Comment on attachment 91120 [details]
patch with changelog and layouttest

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

> third_party/WebKit/LayoutTests/storage/indexeddb/mozilla/readwrite-transactions.html:121
> +    done();

It would be nice if on failure we still called done() so the test doesn't timeout.  One way to do this is to have the onerror callback call done() before calling unexpectedErrorCallback().

[Tony Chang]

Sorry, I didn't see David's comments.  Feel free to fix those and upload a new patch.

[David Grogan]

(In reply to comment #3)
> (From update of attachment 91120 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91120&action=review
> 
> > third_party/WebKit/LayoutTests/storage/indexeddb/mozilla/readwrite-transactions.html:121
> > +    done();
> 
> It would be nice if on failure we still called done() so the test doesn't timeout.  One way to do this is to have the onerror callback call done() before calling unexpectedErrorCallback().

We could have unexpectedErrorCallback call done().  Mark, I think that wouldn't change any expected results and would solve this across all of the IndexedDB tests.

[Mark Pilgrim (Google)]

(In reply to comment #2)
> (From update of attachment 91120 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91120&action=review
> 
> > third_party/WebKit/LayoutTests/storage/indexeddb/mozilla/readwrite-transactions.html:63
> > +    key1 = evalAndLog("key1 = event.target.result;");
> 
> move this to postAdd2, right before key1 is first used
> 
> > third_party/WebKit/LayoutTests/storage/indexeddb/mozilla/readwrite-transactions.html:72
> > +    key2 = evalAndLog("key2 = event.target.result;");
> 
> move this

I can't move these any later because they are gathering values that are dependent on the result of the previous call.

[Mark Pilgrim (Google)]

(In reply to comment #5)
> (In reply to comment #3)
> > It would be nice if on failure we still called done() so the test doesn't timeout.  One way to do this is to have the onerror callback call done() before calling unexpectedErrorCallback().
> 
> We could have unexpectedErrorCallback call done().  Mark, I think that wouldn't change any expected results and would solve this across all of the IndexedDB tests.

I can add this to this bug's next patch.

[David Grogan]

Comment on attachment 91120 [details]
patch with changelog and layouttest

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

>>> third_party/WebKit/LayoutTests/storage/indexeddb/mozilla/readwrite-transactions.html:63
>>> +    key1 = evalAndLog("key1 = event.target.result;");
>> 
>> move this to postAdd2, right before key1 is first used
> 
> I can't move these any later because they are gathering values that are dependent on the result of the previous call.

Oops, I missed that.  Move requests withdrawn.

[David Grogan]

(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > It would be nice if on failure we still called done() so the test doesn't timeout.  One way to do this is to have the onerror callback call done() before calling unexpectedErrorCallback().
> > 
> > We could have unexpectedErrorCallback call done().  Mark, I think that wouldn't change any expected results and would solve this across all of the IndexedDB tests.
> 
> I can add this to this bug's next patch.

Could you file a new bug instead?  Don't feel obligated to make the change.

[Mark Pilgrim (Google)]

Created attachment 91205 [details]
nits addressed

As requested, this does not include a patch to call done() from the error callback.

[Mark Pilgrim (Google)]

(In reply to comment #9)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > (In reply to comment #3)
> > > > It would be nice if on failure we still called done() so the test doesn't timeout.  One way to do this is to have the onerror callback call done() before calling unexpectedErrorCallback().
> > > 
> > > We could have unexpectedErrorCallback call done().  Mark, I think that wouldn't change any expected results and would solve this across all of the IndexedDB tests.
> > 
> > I can add this to this bug's next patch.
> 
> Could you file a new bug instead?  Don't feel obligated to make the change.

Bug 59569

[Mark Pilgrim (Google)]

Created attachment 91214 [details]
rebase webkit directory

[Mark Pilgrim (Google)]

Created attachment 91287 [details]
nits addressed once again (regression in previous patch)

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 91287 [details]:

http/tests/misc/favicon-loads-with-icon-loading-override.html bug 58412 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 91287 [details]
nits addressed once again (regression in previous patch)

Clearing flags on attachment: 91287

Committed r85178: <http://trac.webkit.org/changeset/85178>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.