Bug 69012

Summary: layout test demonstrating IDBCursor inconsistency bug
Product: WebKit Reporter: David Grogan <dgrogan>
Component: New BugsAssignee: David Grogan <dgrogan>
Status: RESOLVED FIXED    
Severity: Normal CC: hans, japhet, jsbell, michaeln, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description David Grogan 2011-09-28 11:58:50 PDT 
layout test demonstrating IDBCursor inconsistency bug
Comment 1 David Grogan 2011-09-28 12:05:22 PDT 
Created attachment 109048 [details]
Patch
Comment 2 David Grogan 2011-09-28 12:21:51 PDT 
Michael, could you review this layout test?  It's the one I mentioned in http://codereview.chromium.org/7834006/.  I can see the inconsistency issue discussion taking a while so want to get this layout test in and out of my local client.
Comment 3 Michael Nordman 2011-09-28 13:04:41 PDT 
Comment on attachment 109048 [details]
Patch

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

> LayoutTests/storage/indexeddb/cursor-inconsistency.html:80
> +    shouldBeEqualToString("storedCursor.key", "someKey" + counter);

would it make sense to have a test for cursor object equality too...

shouldBeEqualTo(storedCursor, event.target.result);
Comment 4 Michael Nordman 2011-09-28 15:31:31 PDT 
Also, this looks like a fairly significant correction bug, if the caller uses the 'wrong' cursor object to retrieve the values, results are completely whacked.

The test lgtm (fwiw, i don't have review rights), but i also think it'd be good to add the stored vs result cursor equality test.
Comment 5 Joshua Bell 2011-09-28 16:03:05 PDT 
Comment on attachment 109048 [details]
Patch

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

> LayoutTests/storage/indexeddb/cursor-inconsistency.html:83
> +    shouldBeEqualToString("event.target.result.value", "someValue" + counter++);

Nit: Just for readability, I'd pull the counter++ onto its own line.
Comment 6 David Grogan 2011-09-28 17:21:12 PDT 
Created attachment 109100 [details]
Patch
Comment 7 David Grogan 2011-09-28 17:21:48 PDT 
Comment on attachment 109048 [details]
Patch

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

>> LayoutTests/storage/indexeddb/cursor-inconsistency.html:80
>> +    shouldBeEqualToString("storedCursor.key", "someKey" + counter);
> 
> would it make sense to have a test for cursor object equality too...
> 
> shouldBeEqualTo(storedCursor, event.target.result);

Added.

>> LayoutTests/storage/indexeddb/cursor-inconsistency.html:83
>> +    shouldBeEqualToString("event.target.result.value", "someValue" + counter++);
> 
> Nit: Just for readability, I'd pull the counter++ onto its own line.

Done
Comment 8 David Grogan 2011-09-28 17:23:19 PDT 
Nate, could you review this test?

(Also I'm now a committer, thanks for the nomination!)
Comment 9 Hans Wennborg 2011-09-29 06:22:32 PDT 
lgtm too; scary stuff
Comment 10 WebKit Review Bot 2011-09-29 09:41:56 PDT 
Comment on attachment 109100 [details]
Patch

Clearing flags on attachment: 109100

Committed r96335: <http://trac.webkit.org/changeset/96335>
Comment 11 WebKit Review Bot 2011-09-29 09:42:00 PDT 
All reviewed patches have been landed.  Closing bug.