Bug 59465

Created attachment 91120 [details] patch with changelog and layouttest

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

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().

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

(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.

(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.

(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.

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.

(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.

Created attachment 91205 [details] nits addressed As requested, this does not include a patch to call done() from the error callback.

(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

Created attachment 91214 [details] rebase webkit directory

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

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.

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>

All reviewed patches have been landed. Closing bug.