Bug 59465

Summary: Port Mozilla's IndexedDB tests: readonly and read/write transactions
Product: WebKit Reporter: Mark Pilgrim (Google) <pilgrim>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dgrogan, fishd, hans, pilgrim, tony
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
patch with changelog and layouttest
tony: review+
nits addressed
none
rebase webkit directory
none
nits addressed once again (regression in previous patch) none

Mark Pilgrim (Google)
Reported 2011-04-26 09:47:19 PDT
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.
Attachments
patch with changelog and layouttest (13.80 KB, patch)
2011-04-26 09:49 PDT, Mark Pilgrim (Google)
tony: review+
nits addressed (14.16 KB, patch)
2011-04-26 18:30 PDT, Mark Pilgrim (Google)
no flags
rebase webkit directory (13.52 KB, patch)
2011-04-26 19:38 PDT, Mark Pilgrim (Google)
no flags
nits addressed once again (regression in previous patch) (13.52 KB, patch)
2011-04-27 08:33 PDT, Mark Pilgrim (Google)
no flags
Mark Pilgrim (Google)
Comment 1 2011-04-26 09:49:59 PDT
Created attachment 91120 [details] patch with changelog and layouttest
David Grogan
Comment 2 2011-04-26 16:16:24 PDT
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 3 2011-04-26 16:19:28 PDT
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
Comment 4 2011-04-26 16:21:00 PDT
Sorry, I didn't see David's comments. Feel free to fix those and upload a new patch.
David Grogan
Comment 5 2011-04-26 16:26:48 PDT
(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)
Comment 6 2011-04-26 17:15:20 PDT
(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)
Comment 7 2011-04-26 17:17:04 PDT
(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 8 2011-04-26 17:20:26 PDT
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
Comment 9 2011-04-26 17:22:54 PDT
(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)
Comment 10 2011-04-26 18:30:30 PDT
Created attachment 91205 [details] nits addressed As requested, this does not include a patch to call done() from the error callback.
Mark Pilgrim (Google)
Comment 11 2011-04-26 18:47:08 PDT
(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)
Comment 12 2011-04-26 19:38:36 PDT
Created attachment 91214 [details] rebase webkit directory
Mark Pilgrim (Google)
Comment 13 2011-04-27 08:33:09 PDT
Created attachment 91287 [details] nits addressed once again (regression in previous patch)
WebKit Commit Bot
Comment 14 2011-04-28 05:57:30 PDT
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: alice.liu@apple.com) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 15 2011-04-28 05:58:36 PDT
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
Comment 16 2011-04-28 05:58:42 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.