Bug 59569

Summary: Should call done() from unexpectedErrorCallback so failing IndexedDB tests don't time out
Product: WebKit Reporter: Mark Pilgrim (Google) <pilgrim>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dgrogan, fishd, pilgrim, tony
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
diff of affected test
none
Patch none

Description Mark Pilgrim (Google) 2011-04-26 18:35:43 PDT
As discussed in bug 59465, especially https://bugs.webkit.org/show_bug.cgi?id=59465#c9
Comment 1 Mark Pilgrim (Google) 2011-04-26 18:38:22 PDT
Presumably also from unexpectedSuccessCallback, unexpectedAbortCallback, unexpectedCompleteCallback, and unexpectedBlockedCallback.

These are all in LayoutTests/storage/indexeddb/resources/shared.js
Comment 2 Mark Pilgrim (Google) 2011-04-26 18:43:38 PDT
Preliminary test run with these changes (calling done() in all the unexpected callbacks) confirms that this does not affect existing tests... except one. LayoutTests/storage/indexeddb/exception-in-event-aborts.html shows a slight difference. Attaching diff for discussion.
Comment 3 Mark Pilgrim (Google) 2011-04-26 18:45:39 PDT
Created attachment 91207 [details]
diff of affected test
Comment 4 David Grogan 2011-04-26 20:07:50 PDT
Comment on attachment 91207 [details]
diff of affected test

That test needs some fixing, but I'm not yet sure how much.  I think 

request = evalAndLog("store.add({x: 'value4', y: 'zzz4'}, 'key4')");
request.onsuccess = unexpectedSuccessCallback;
request.onerror = throwAndCatch;

should probably be changed to 

request.onsuccess = function(){ testPassed("something") }
request.onerror = unexpectedErrorCallback

but I'm not yet positive that Jeremy made a mistake as opposed to me not understanding his cleverness.
Comment 5 Mark Pilgrim (Google) 2011-04-27 15:06:14 PDT
I'm going ahead with this change as well as your proposed fix for exception-in-event-aborts.html.
Comment 6 Mark Pilgrim (Google) 2011-04-27 15:08:05 PDT
Created attachment 91356 [details]
Patch
Comment 7 David Grogan 2011-04-27 15:14:20 PDT
Comment on attachment 91356 [details]
Patch

r+

Out of curiosity, and for my own sake, how are you updating the text expectation txt files?  I copy them manually from layout-test-results but I'd imagine there's a better way.
Comment 8 Tony Chang 2011-04-27 15:16:14 PDT
BTW, normally when uploading a patch, you should set cq? if you want me to also cq+ the patch.  I've been adding cq+ since you guys don't have commit access right now, but cq? makes it more explicit that you want me to do this.
Comment 9 Mark Pilgrim (Google) 2011-04-27 18:41:30 PDT
(In reply to comment #7)
> Out of curiosity, and for my own sake, how are you updating the text expectation txt files?  I copy them manually from layout-test-results but I'd imagine there's a better way.

That's what I'm doing too, plus re-running them to make sure they pass and I didn't mess up the whitespace or anything.
Comment 10 WebKit Commit Bot 2011-04-27 22:18:48 PDT
Comment on attachment 91356 [details]
Patch

Clearing flags on attachment: 91356

Committed r85153: <http://trac.webkit.org/changeset/85153>
Comment 11 WebKit Commit Bot 2011-04-27 22:18:53 PDT
All reviewed patches have been landed.  Closing bug.