Bug 102415 - IndexedDB setVersion-removal batch 7
Summary: IndexedDB setVersion-removal batch 7
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Grogan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-15 11:40 PST by David Grogan
Modified: 2012-11-15 16:30 PST (History)
4 users (show)

See Also:

Attachments
Patch (40.73 KB, patch)
2012-11-15 11:50 PST, David Grogan 		no flags Details | Formatted Diff | Diff
Patch (41.03 KB, patch)
2012-11-15 15:10 PST, David Grogan 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description David Grogan 2012-11-15 11:40:57 PST 
IndexedDB setVersion-removal batch 7
Comment 1 David Grogan 2012-11-15 11:50:19 PST 
Created attachment 174495 [details]
Patch
Comment 2 David Grogan 2012-11-15 11:52:16 PST 
Josh, could you look at this batch of 6-7?  After this one there are 25 tests left.
Comment 3 Joshua Bell 2012-11-15 15:01:00 PST 
Comment on attachment 174495 [details]
Patch

lgtm with nits (although my eyes are glazing over)

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

> LayoutTests/storage/indexeddb/mozilla/resources/versionchange-abort.js:28
> +    trans.oncomplete = unexpectedSuccessCallback;

Should be unexpectedCompleteCallback

> LayoutTests/storage/indexeddb/resources/objectstore-basics.js:78
> +    request.onerror = addData;

addData() is only able to continue with the test because the aborted "versionchange" transaction doesn't close the database.

Can you add a FIXME referencing the existing bug on that? This test will probably need to be restructured slightly (i.e. open a new connection in addData)

> LayoutTests/storage/indexeddb/resources/shared.js:197
> +        shouldBe("openRequest.readyState", "'pending'", true/*quiet*/);

You may need to add delete self.openRequest at the end of the end of the function, otherwise the reference would "leak" and could affect tests that try and gc()? (They probably shouldn't use this method anyway, but good hygiene to not leave globals around.)
Comment 4 David Grogan 2012-11-15 15:07:29 PST 
Comment on attachment 174495 [details]
Patch

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

>> LayoutTests/storage/indexeddb/mozilla/resources/versionchange-abort.js:28
>> +    trans.oncomplete = unexpectedSuccessCallback;
> 
> Should be unexpectedCompleteCallback

Fixed.

>> LayoutTests/storage/indexeddb/resources/objectstore-basics.js:78
>> +    request.onerror = addData;
> 
> addData() is only able to continue with the test because the aborted "versionchange" transaction doesn't close the database.
> 
> Can you add a FIXME referencing the existing bug on that? This test will probably need to be restructured slightly (i.e. open a new connection in addData)

FIXME added.

>> LayoutTests/storage/indexeddb/resources/shared.js:197
>> +        shouldBe("openRequest.readyState", "'pending'", true/*quiet*/);
> 
> You may need to add delete self.openRequest at the end of the end of the function, otherwise the reference would "leak" and could affect tests that try and gc()? (They probably shouldn't use this method anyway, but good hygiene to not leave globals around.)

Fixed.
Comment 5 David Grogan 2012-11-15 15:10:18 PST 
Created attachment 174530 [details]
Patch
Comment 6 David Grogan 2012-11-15 15:11:01 PST 
Comment on attachment 174530 [details]
Patch

Tony, could review this?
Comment 7 David Grogan 2012-11-15 16:12:36 PST 
Tony, could you review this?  (I suppose it'd be helpful to actually cc you :)
Comment 8 WebKit Review Bot 2012-11-15 16:29:58 PST 
Comment on attachment 174530 [details]
Patch

Clearing flags on attachment: 174530

Committed r134854: <http://trac.webkit.org/changeset/134854>
Comment 9 WebKit Review Bot 2012-11-15 16:30:01 PST 
All reviewed patches have been landed.  Closing bug.