Bug 55376

Summary: IndexedDB: Move SQL code, especially for cursors, to IDBBackingStore
Product: WebKit Reporter: Hans Wennborg <hans>
Component: New BugsAssignee: Hans Wennborg <hans>
Status: RESOLVED FIXED    
Severity: Normal CC: andreip, dgrogan, jorlow
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on: 55443, 55692    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch jorlow: review+

Hans Wennborg
Reported 2011-02-28 07:40:05 PST
IndexedDB: Move SQL code, especially for cursors, to IDBBackingStore
Attachments
Patch (60.95 KB, patch)
2011-02-28 07:50 PST, Hans Wennborg
no flags
Patch (58.55 KB, patch)
2011-03-02 07:19 PST, Hans Wennborg
jorlow: review+
Hans Wennborg
Comment 1 2011-02-28 07:50:54 PST
Hans Wennborg
Comment 2 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.
Jeremy Orlow
Comment 3 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
Hans Wennborg
Comment 4 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.
Hans Wennborg
Comment 5 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.
Hans Wennborg
Comment 6 2011-03-02 07:19:15 PST
Jeremy Orlow
Comment 7 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?
Hans Wennborg
Comment 8 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?
Hans Wennborg
Comment 9 2011-03-03 02:38:01 PST
Note You need to log in before you can comment on or make changes to this bug.