Bug 55376 - IndexedDB: Move SQL code, especially for cursors, to IDBBackingStore
Summary: IndexedDB: Move SQL code, especially for cursors, to IDBBackingStore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Hans Wennborg
URL:
Keywords:
Depends on: 55443 55692
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-28 07:40 PST by Hans Wennborg
Modified: 2011-03-03 11:21 PST (History)
3 users (show)

See Also:


Attachments
Patch (60.95 KB, patch)
2011-02-28 07:50 PST, Hans Wennborg
no flags Details | Formatted Diff | Diff
Patch (58.55 KB, patch)
2011-03-02 07:19 PST, Hans Wennborg
jorlow: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hans Wennborg 2011-02-28 07:40:05 PST
IndexedDB: Move SQL code, especially for cursors, to IDBBackingStore
Comment 1 Hans Wennborg 2011-02-28 07:50:54 PST
Created attachment 84060 [details]
Patch
Comment 2 Hans Wennborg 2011-02-28 07:55:37 PST
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.
Comment 3 Jeremy Orlow 2011-02-28 20:29:58 PST
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
Comment 4 Hans Wennborg 2011-03-01 07:49:08 PST
(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.
Comment 5 Hans Wennborg 2011-03-02 07:18:46 PST
Uploading new patch rebased on top of Jeremy's patch in Bug 55443, and addressing Jeremy's comments.

Expecting purple ews bots.
Comment 6 Hans Wennborg 2011-03-02 07:19:15 PST
Created attachment 84417 [details]
Patch
Comment 7 Jeremy Orlow 2011-03-02 15:51:46 PST
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?
Comment 8 Hans Wennborg 2011-03-03 02:32:31 PST
(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?
Comment 9 Hans Wennborg 2011-03-03 02:38:01 PST
Committed r80220: <http://trac.webkit.org/changeset/80220>