[webkit-reviews] review denied: [Bug 55376] IndexedDB: Move SQL code, especially for cursors, to IDBBackingStore : [Attachment 84060] Patch

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has denied Hans Wennborg
<hans at chromium.org>'s request for review:
Bug 55376: IndexedDB: Move SQL code, especially for cursors, to IDBBackingStore
https://bugs.webkit.org/show_bug.cgi?id=55376

Attachment 84060: Patch
https://bugs.webkit.org/attachment.cgi?id=84060&action=review

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=84060&action=review

*sigh*...I should have read this before I started working on my patch.	Our
work is majorly overlapping.  I guess it's a good thing that we made a lot of
very similar changes though.

Please make these changes and otherwise try to make it look as similar to
https://bugs.webkit.org/show_bug.cgi?id=55443 as possible.  If you do that and
get Andrei to sign off on it then r=me.  Otherwise I'll try to review it early
tomorrow if possible.

> Source/WebCore/storage/IDBBackingStore.cpp:705
> +	   if (key && !key->isEqual(m_currentKey.get()))

Uh oh...this is wrong.	We're supposed to loop while less than, and stop if
something is greater than or equal.  We need to fix this for m11.

> Source/WebCore/storage/IDBBackingStore.h:79
> +    class Cursor : public RefCounted<Cursor> {

Like I mentioned elsewhere, I'm not sure it makes sense to have all of these
sub-classes...I think it just makes life more difficult for us.

> Source/WebCore/storage/IDBCursorBackendImpl.cpp:64
> +    switch (m_cursorType) {

This is ugly...there's no way to do some template magic or something?  Why
can't we just have one interface and have stuff assert we don't call methods
that don't exist?  (That's what I did in my patch.)

> Source/WebCore/storage/IDBCursorBackendImpl.h:53
> +    enum CursorType {

Put on IDBCursorBackendInterface