Bug 74705 – IndexedDB: IDBKeyRange constructor should throw when lower > upper

• Component: New Bugs
• Status: RESOLVED FIXED

[Hans Wennborg]

IndexedDB: IDBKeyRange constructor should throw when lower > upper

[Hans Wennborg]

Created attachment 119594 [details]
Patch

[Hans Wennborg]

Ping?

[Tony Chang]

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?

[Hans Wennborg]

Created attachment 120297 [details]
Patch

[Hans Wennborg]

(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?

[Joshua Bell]

Comment on attachment 120297 [details]
Patch

count test changes LGTM

[Joshua Bell]

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.)

[Hans Wennborg]

(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!

[Hans Wennborg]

Created attachment 120584 [details]
Patch

[WebKit Review Bot]

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

[Joshua Bell]

Comment on attachment 120584 [details]
Patch

LGTM

[WebKit Review Bot]

Comment on attachment 120584 [details]
Patch

Clearing flags on attachment: 120584

Committed r103762: <http://trac.webkit.org/changeset/103762>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.