Summary: | [IndexedDB] upperOpen set to true in lowerBound()/lowerOpen set to true in upperBound() | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Xingnan Wang <xingnan.wang> | ||||||||||
Component: | New Bugs | Assignee: | Xingnan Wang <xingnan.wang> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | dgrogan, jochen, jsbell, tony, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Xingnan Wang
2012-07-10 02:41:41 PDT
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 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. > 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. :)
Created attachment 151573 [details]
Patch
(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 tony@ - could you r? 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. Created attachment 151839 [details]
Patch
(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 Created attachment 151875 [details]
Patch
(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 on attachment 151875 [details] Patch Clearing flags on attachment: 151875 Committed r122435: <http://trac.webkit.org/changeset/122435> All reviewed patches have been landed. Closing bug. |