Bug 74705 - IndexedDB: IDBKeyRange constructor should throw when lower > upper
Summary: IndexedDB: IDBKeyRange constructor should throw when lower > upper
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: Hans Wennborg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-16 03:36 PST by Hans Wennborg
Modified: 2011-12-28 04:59 PST (History)
5 users (show)

See Also:


Attachments
Patch (13.71 KB, patch)
2011-12-16 03:40 PST, Hans Wennborg
no flags Details | Formatted Diff | Diff
Patch (17.82 KB, patch)
2011-12-22 03:00 PST, Hans Wennborg
no flags Details | Formatted Diff | Diff
Patch (17.83 KB, patch)
2011-12-27 06:52 PST, Hans Wennborg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hans Wennborg 2011-12-16 03:36:48 PST
IndexedDB: IDBKeyRange constructor should throw when lower > upper
Comment 1 Hans Wennborg 2011-12-16 03:40:06 PST
Created attachment 119594 [details]
Patch
Comment 2 Hans Wennborg 2011-12-20 00:50:17 PST
Ping?
Comment 3 Tony Chang 2011-12-20 09:53:23 PST
Comment on attachment 119594 [details]
Patch

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

> LayoutTests/storage/indexeddb/keyrange.html:197
> +    try {
> +        debug("Equal keys, either of the bounds is open, bound(4, 4, true, true)");

Maybe add a (4, 4, false, false) test for completeness?
Comment 4 Hans Wennborg 2011-12-22 03:00:15 PST
Created attachment 120297 [details]
Patch
Comment 5 Hans Wennborg 2011-12-22 03:01:28 PST
(In reply to comment #3)
> Maybe add a (4, 4, false, false) test for completeness?
Done.

Josh: I had to adjust the layout tests for the count() method. Could you take a look?
Comment 6 Joshua Bell 2011-12-22 11:13:39 PST
Comment on attachment 120297 [details]
Patch

count test changes LGTM
Comment 7 Joshua Bell 2011-12-22 16:09:59 PST
Comment on attachment 120297 [details]
Patch

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

... and overall LTGM with nits.

> Source/WebCore/ChangeLog:9
> +        or lower == upper and one of the bounds is open.

Nit: one or both of the bounds is open

> LayoutTests/storage/indexeddb/keyrange.html:178
> +        testFailed("No exception thrown");

I know this follows the pattern of other tests in the file, but these should really be using evalAndExpectException() and verifying that DATA_ERR was the indeed the error thrown. (Verifying the error type is important as we've unintentionally shifted responsibility to the binding layer in the past and changed the exception type without catching it in tests.)
Comment 8 Hans Wennborg 2011-12-27 06:51:39 PST
(In reply to comment #7)
> (From update of attachment 120297 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120297&action=review
> 
> ... and overall LTGM with nits.
> 
> > Source/WebCore/ChangeLog:9
> > +        or lower == upper and one of the bounds is open.
> 
> Nit: one or both of the bounds is open
Done.

> 
> > LayoutTests/storage/indexeddb/keyrange.html:178
> > +        testFailed("No exception thrown");
> 
> I know this follows the pattern of other tests in the file, but these should really be using evalAndExpectException() and verifying that DATA_ERR was the indeed the error thrown. (Verifying the error type is important as we've unintentionally shifted responsibility to the binding layer in the past and changed the exception type without catching it in tests.)
Done.

New patch coming up!
Comment 9 Hans Wennborg 2011-12-27 06:52:08 PST
Created attachment 120584 [details]
Patch
Comment 10 WebKit Review Bot 2011-12-27 07:37:29 PST
Comment on attachment 120584 [details]
Patch

Attachment 120584 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11041145

New failing tests:
http/tests/inspector/resource-tree/resource-tree-document-url.html
Comment 11 Joshua Bell 2011-12-27 11:18:33 PST
Comment on attachment 120584 [details]
Patch

LGTM
Comment 12 WebKit Review Bot 2011-12-28 04:59:00 PST
Comment on attachment 120584 [details]
Patch

Clearing flags on attachment: 120584

Committed r103762: <http://trac.webkit.org/changeset/103762>
Comment 13 WebKit Review Bot 2011-12-28 04:59:05 PST
All reviewed patches have been landed.  Closing bug.