Bug 90867 - [IndexedDB] upperOpen set to true in lowerBound()/lowerOpen set to true in upperBound()
Summary: [IndexedDB] upperOpen set to true in lowerBound()/lowerOpen set to true in up...
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: Xingnan Wang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-10 02:41 PDT by Xingnan Wang
Modified: 2012-07-12 02:47 PDT (History)
5 users (show)

See Also:


Attachments
Patch (7.89 KB, patch)
2012-07-10 02:43 PDT, Xingnan Wang
no flags Details | Formatted Diff | Diff
Patch (7.75 KB, patch)
2012-07-10 18:45 PDT, Xingnan Wang
no flags Details | Formatted Diff | Diff
Patch (7.78 KB, patch)
2012-07-11 18:28 PDT, Xingnan Wang
no flags Details | Formatted Diff | Diff
Patch (7.84 KB, patch)
2012-07-12 00:47 PDT, Xingnan Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Xingnan Wang 2012-07-10 02:43:22 PDT
Created attachment 151423 [details]
Patch
Comment 2 jochen 2012-07-10 03:44:11 PDT
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
Comment 3 Joshua Bell 2012-07-10 09:23:40 PDT
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.
Comment 4 Joshua Bell 2012-07-10 09:24:17 PDT
> 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. :)
Comment 5 Xingnan Wang 2012-07-10 18:45:52 PDT
Created attachment 151573 [details]
Patch
Comment 6 Xingnan Wang 2012-07-10 18:49:28 PDT
(In reply to comment #5)
> Created an attachment (id=151573) [details]
> Patch

Thanks for your review, patch updated as the comments.
Comment 7 jochen 2012-07-11 00:56:42 PDT
This patch looks good to me
Comment 8 Joshua Bell 2012-07-11 14:30:56 PDT
tony@ - could you r?
Comment 9 Tony Chang 2012-07-11 15:26:32 PDT
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.
Comment 10 Xingnan Wang 2012-07-11 18:28:59 PDT
Created attachment 151839 [details]
Patch
Comment 11 Xingnan Wang 2012-07-11 18:30:33 PDT
(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 12 jochen 2012-07-12 00:36:46 PDT
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
Comment 13 Xingnan Wang 2012-07-12 00:47:38 PDT
Created attachment 151875 [details]
Patch
Comment 14 Xingnan Wang 2012-07-12 00:48:15 PDT
(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.
Comment 15 WebKit Review Bot 2012-07-12 02:47:12 PDT
Comment on attachment 151875 [details]
Patch

Clearing flags on attachment: 151875

Committed r122435: <http://trac.webkit.org/changeset/122435>
Comment 16 WebKit Review Bot 2012-07-12 02:47:17 PDT
All reviewed patches have been landed.  Closing bug.