Bug 44546 – Implement the rest of IDBCursors + make them persistent

• Component: New Bugs
• Status: RESOLVED FIXED

[Jeremy Orlow]

Implement the rest of IDBCursors + make them persistent

[Jeremy Orlow]

Created attachment 65311 [details]
Patch

[Jeremy Orlow]

Don't worry, the bulk of the size of this patch is just a test.

[Andrei Popescu]

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?

[Jeremy Orlow]

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

[Jeremy Orlow]

Created attachment 65415 [details]
Patch

[Andrei Popescu]

LGTM, nice work.

[Steve Block]

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?

[Jeremy Orlow]

(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);"

[Jeremy Orlow]

Created attachment 65722 [details]
Patch

[Steve Block]

Comment on attachment 65722 [details]
Patch

r=me

[Jeremy Orlow]

Committed r66473: <http://trac.webkit.org/changeset/66473>

[WebKit Review Bot]

http://trac.webkit.org/changeset/66473 might have broken Chromium Mac Release