Summary: | Implement the rest of IDBCursors + make them persistent | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jeremy Orlow <jorlow> | ||||||||
Component: | New Bugs | Assignee: | Jeremy Orlow <jorlow> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, andreip, bulach, eric, steveblock, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Other | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Jeremy Orlow
2010-08-24 13:00:30 PDT
Created attachment 65311 [details]
Patch
Don't worry, the bulk of the size of this patch is just a test. Looks good, couple of comments: > static String cursorWhereFragment(IDBKey::Type type, bool lowerBound, String equality) equality isn't always equality. Maybe call it condition? > "? " + equality + " keyString AND keyDate IS NULL AND keyNumber IS NULL AND "; Hmm, a tuple cannot simultaneously have values for more than one key type, can it? Given this, isn't it sufficient to just match the appropriate column here and not worry about the rest being NULL? (In reply to comment #3) > Looks good, couple of comments: > > > static String cursorWhereFragment(IDBKey::Type type, bool lowerBound, String equality) > > equality isn't always equality. Maybe call it condition? Will do. > > "? " + equality + " keyString AND keyDate IS NULL AND keyNumber IS NULL AND "; > > Hmm, a tuple cannot simultaneously have values for more than one key type, can it? Given this, isn't it sufficient to just match the appropriate column here and not worry about the rest being NULL? I guess so. Steve can you take a look? (Or scan for style and rubber stamp based on Andrei's review since I think it's safe to say he's an expert on this code, even though he's not an official reviewer yet.) Created attachment 65415 [details]
Patch
LGTM, nice work. Comment on attachment 65415 [details]
Patch
WebCore/storage/IDBObjectStoreBackendImpl.cpp:269
+ static String cursorWhereFragment(IDBKey::Type type, bool lowerBound, String comparisonOperator)
this would be easier to follow, both at call site and impl, if it were split into two methods - 'cursorWhereFragmentLowerBound' and 'cursorWhereFragmentUppperBound'. I don't think the current use of a bool saves us any code and it makes it harder to follow.
WebCore/storage/IDBObjectStoreBackendImpl.cpp:299
+ void IDBObjectStoreBackendImpl::openCursor(PassRefPtr<IDBKeyRange> range, unsigned short tmpDirection, PassRefPtr<IDBCallbacks> callbacks)
Why is the direction passed in as an unsigned short?
(In reply to comment #7) > (From update of attachment 65415 [details]) > WebCore/storage/IDBObjectStoreBackendImpl.cpp:269 > + static String cursorWhereFragment(IDBKey::Type type, bool lowerBound, String comparisonOperator) > this would be easier to follow, both at call site and impl, if it were split into two methods - 'cursorWhereFragmentLowerBound' and 'cursorWhereFragmentUppperBound'. I don't think the current use of a bool saves us any code and it makes it harder to follow. Good thought! Done. > WebCore/storage/IDBObjectStoreBackendImpl.cpp:299 > + void IDBObjectStoreBackendImpl::openCursor(PassRefPtr<IDBKeyRange> range, unsigned short tmpDirection, PassRefPtr<IDBCallbacks> callbacks) > Why is the direction passed in as an unsigned short? Because that's what's in the IDL? "IDBRequest openCursor (in optional IDBKeyRange range, in optional unsigned short direction) raises (IDBDatabaseException);" Created attachment 65722 [details]
Patch
Comment on attachment 65722 [details]
Patch
r=me
Committed r66473: <http://trac.webkit.org/changeset/66473> http://trac.webkit.org/changeset/66473 might have broken Chromium Mac Release |