Bug 55376 – IndexedDB: Move SQL code, especially for cursors, to IDBBackingStore

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

[Hans Wennborg]

IndexedDB: Move SQL code, especially for cursors, to IDBBackingStore

[Hans Wennborg]

Created attachment 84060 [details]
Patch

[Hans Wennborg]

This is another fat patch, but again, most of it is just code being moved around. I believe this should be the last fat patch in this refactoring.

I expect function signatures and the classes concerning cursors are most interesting to focus on.

[Jeremy Orlow]

Comment on attachment 84060 [details]
Patch

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

[Hans Wennborg]

(In reply to comment #3)
> (From update of attachment 84060 [details])
> 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.


No worries, I'll let your patch land and then rebase off of it. I don't think there's any reason to rush this.

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

Ah, well spotted. I noticed you filed crbug.com/74500 for this.

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

Yeah, as noted below, I'll put it all in a common interface.

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

Yes, I'll do that. Your patch fixes the problem that value() could return different types (a value or a key) depending on cursor type, so this should all work out nicely.

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

Cool, I'll just use that.

[Hans Wennborg]

Uploading new patch rebased on top of Jeremy's patch in Bug 55443, and addressing Jeremy's comments.

Expecting purple ews bots.

[Hans Wennborg]

Created attachment 84417 [details]
Patch

[Jeremy Orlow]

Comment on attachment 84417 [details]
Patch

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

r=me

> Source/WebCore/storage/IDBBackingStore.cpp:669
> +class CursorImplCommon : public IDBBackingStore::Cursor {

I feel like all of this is a bit overly complicated.  You're still joining on index data for index.keyCursors, which is the primary reason I could imagine this logic becoming more complicated.  Maybe take a stab at simplifying a bit?

[Hans Wennborg]

(In reply to comment #7)
> (From update of attachment 84417 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84417&action=review
> 
> r=me
> 
> > Source/WebCore/storage/IDBBackingStore.cpp:669
> > +class CursorImplCommon : public IDBBackingStore::Cursor {
> 
> I feel like all of this is a bit overly complicated.  You're still joining on index data for index.keyCursors, which is the primary reason I could imagine this logic becoming more complicated.  Maybe take a stab at simplifying a bit?

I'll land this now, and try to simplify a bit in a follow-up patch.

Which part is it that you find complicated? That there are different classes for the different kinds of cursors? Or just that they could share code in a more elegant way?

[Hans Wennborg]

Committed r80220: <http://trac.webkit.org/changeset/80220>