Bug 90867 – [IndexedDB] upperOpen set to true in lowerBound()/lowerOpen set to true in upperBound()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Xingnan Wang]

Keep align with the spec
http://www.w3.org/TR/IndexedDB/#widl-IDBKeyRange-lowerBound-static-IDBKeyRange-any-lower-boolean-open

[Xingnan Wang]

Created attachment 151423 [details]
Patch

Comment on attachment 151423 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

can you please address this?

Esp. give a justification why this change is required?

> Source/WebCore/ChangeLog:10
> +        No new tests (OOPS!).

You should remove this line

[Joshua Bell]

Nice catch - thanks for noticing the issue and providing the patch.

Looking through the IDB code and this patch, the patch and test updates look good to me.

[Joshua Bell]

> Looking through the IDB code and this patch, the patch and test updates look good to me.

Apart from jochen@'s feedback on the ChangeLogs, of course. :)

[Xingnan Wang]

Created attachment 151573 [details]
Patch

[Xingnan Wang]

(In reply to comment #5)
> Created an attachment (id=151573) [details]
> Patch

Thanks for your review, patch updated as the comments.

This patch looks good to me

[Joshua Bell]

tony@ - could you r?

[Tony Chang]

Comment on attachment 151573 [details]
Patch

(In reply to comment #2)
> (From update of attachment 151423 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151423&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        No new tests (OOPS!).
> 
> You should remove this line

Actually, you should replace it with a line explaining which tests cover this change or why it can't be tested.  For this change, it should say something like:
  No new tests - updated storage/indexeddb/keyrange.html to match new behavior.

[Xingnan Wang]

Created attachment 151839 [details]
Patch

[Xingnan Wang]

(In reply to comment #9)
> (From update of attachment 151573 [details])
> (In reply to comment #2)
> > (From update of attachment 151423 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=151423&action=review
> > 
> > > Source/WebCore/ChangeLog:10
> > > +        No new tests (OOPS!).
> > 
> > You should remove this line
> 
> Actually, you should replace it with a line explaining which tests cover this change or why it can't be tested.  For this change, it should say something like:
>   No new tests - updated storage/indexeddb/keyrange.html to match new behavior.

Got it, updated the patch added this line.

Comment on attachment 151839 [details]
Patch

Since Tony already reviewed this, you don't need to request another review

If you fix below nit, I can set cq+

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

> Source/WebCore/ChangeLog:10
> +        No new tests - updated storage/indexeddb/keyrange.html to match new behavior.

nit. I would expect an empty line before this one

[Xingnan Wang]

Created attachment 151875 [details]
Patch

[Xingnan Wang]

(In reply to comment #12)
> (From update of attachment 151839 [details])
> Since Tony already reviewed this, you don't need to request another review
> 
> If you fix below nit, I can set cq+
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=151839&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        No new tests - updated storage/indexeddb/keyrange.html to match new behavior.
> 
> nit. I would expect an empty line before this one

Done.

Thank you.

[WebKit Review Bot]

Comment on attachment 151875 [details]
Patch

Clearing flags on attachment: 151875

Committed r122435: <http://trac.webkit.org/changeset/122435>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.