[At fishd's suggestion, I'm splitting up the test suite in bug 58560 and getting them reviewed in chunks] Original test: http://mxr.mozilla.org/mozilla2.0/source/dom/indexedDB/test/test_setVersion_events.html?force=1 This is a port of the Mozilla IndexedDB test for the versionchange event. It tests whether the versionchange event is triggered when multiple connections to the same database are open and one of them calls setVersion. WebKit passes this test already. Passing LayoutTest and expected output file attached. This is a review-ready patch.
Created attachment 89937 [details] LayoutTest + expected output file
Comment on attachment 89937 [details] LayoutTest + expected output file View in context: https://bugs.webkit.org/attachment.cgi?id=89937&action=review > third_party/WebKit/LayoutTests/storage/indexeddb/mozilla/versionchange.html:42 > + // db1 will be open when we call db2.setVersion, which will trigger this Put this in a debug(""). Copy/pasted comment from jorlow that's applicable: "Stuff like this should be in shouldBe/testPassed/debug()'s. The idea is that you should be able to follow along via the on-screen output." > third_party/WebKit/LayoutTests/storage/indexeddb/mozilla/versionchange.html:62 > + shouldBe("event.target.version", "'1'"); Puzzled at the nested quotes - do we need them to pass the shouldBe assert? Have I just never noticed them elsewhere in our tests? > third_party/WebKit/LayoutTests/storage/indexeddb/mozilla/versionchange.html:70 > + request.onsuccess = postSetVersion; Can you add a listener for request.onblocked? UnexpectedBlockCallback or function() { testPassed("db2 received blocked event") } would be sufficient. I think we will fire a blocked event at db2 in spite of the call to db1.close, but I remember the spec being unclear on this point. Do you know what firefox does? > third_party/WebKit/LayoutTests/storage/indexeddb/mozilla/versionchange.html:89 > + // this will block because db2 is still open Yeah, put this in debug("") inside the onblocked function
Created attachment 90061 [details] LayoutTest + expected output file Put all comments into debug() statements instead and updated expected output file to match. Added listener for request.onblocked as requested. Yes, we call it one time even though versionchange calls db1.close. Mozilla does not call it at all. Not sure whose bug it is. Can file a separate bug to track it if desired. For now I put in a testPassed() call to document it. Nested quotes are necessary when the string being compared is a constant. In this case, '1', not 1. If I remove the nested quotes, the assertion fails because it treats 1 as a number and complains of a type mismatch.
Unofficial r+. (In reply to comment #3) > Created an attachment (id=90061) [details] > LayoutTest + expected output file > Added listener for request.onblocked as requested. Yes, we call it one time even though versionchange calls db1.close. Mozilla does not call it at all. Not sure whose bug it is. Can file a separate bug to track it if desired. For now I put in a testPassed() call to document it. Thanks. I think it's our bug, the versionchangeevent is probably supposed to be synchronous, not asynchronous as we treat it now. I'd appreciate it if you filed a bug pointing to https://bugs.webkit.org/attachment.cgi?id=90061 as a test case but don't worry about it if you're swamped.
Andrei, would you be willing to review this?
Comment on attachment 90061 [details] LayoutTest + expected output file This patch needs a ChangeLog entry. R- Please see http://www.webkit.org/coding/contributing.html for guidance.
Created attachment 90958 [details] patch with changelog and layouttest added changelog to patch
Unofficial r+
Comment on attachment 90958 [details] patch with changelog and layouttest Rejecting attachment 90958 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-sf-01', 'app..." exit_code: 2 Last 500 characters of output: tTests/ChangeLog (revision 84805) |+++ third_party/WebKit/LayoutTests/ChangeLog (working copy) -------------------------- No file to patch. Skipping patch. 1 out of 1 hunk ignored patching file third_party/WebKit/LayoutTests/storage/indexeddb/mozilla/versionchange-expected.txt patching file third_party/WebKit/LayoutTests/storage/indexeddb/mozilla/versionchange.html Failed to run "[u'/Users/abarth/git/webkit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Darin Fisher', u'--f..." exit_code: 1 Full output: http://queues.webkit.org/results/8510791
Created attachment 91213 [details] rebase webkit directory
Comment on attachment 91213 [details] rebase webkit directory missing the ChangeLog :(
Created attachment 91285 [details] nits addressed once again (regression in previous patch)
Comment on attachment 91285 [details] nits addressed once again (regression in previous patch) Rejecting attachment 91285 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'land-a..." exit_code: 1 Last 500 characters of output: autoinstalled/mechanize/_urllib2_fork.py", line 332, in _call_chain result = func(*args) File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1170, in https_open return self.do_open(conn_factory, req) File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1118, in do_open raise URLError(err) urllib2.URLError: <urlopen error [Errno 60] Operation timed out> Full output: http://queues.webkit.org/results/8514456
Comment on attachment 91285 [details] nits addressed once again (regression in previous patch) commit queue hit a transient timeout, kicking to try again
Comment on attachment 91285 [details] nits addressed once again (regression in previous patch) Clearing flags on attachment: 91285 Committed r85232: <http://trac.webkit.org/changeset/85232>
All reviewed patches have been landed. Closing bug.