[webkit-reviews] review denied: [Bug 44164] Implement persistance for IndexedDB ObjectStores : [Attachment 64727] Patch

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has denied Jeremy Orlow
<jorlow at chromium.org>'s request for review:
Bug 44164: Implement persistance for IndexedDB ObjectStores
https://bugs.webkit.org/show_bug.cgi?id=44164

Attachment 64727: Patch
https://bugs.webkit.org/attachment.cgi?id=64727&action=review

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
(In reply to comment #12)
> (From update of attachment 64727 [details])
> WebCore/storage/IDBFactoryBackendImpl.cpp:81
>  +	      "CREATE TABLE IF NOT EXISTS MetaData (id INTEGER PRIMARY KEY,
name TEXT, description TEXT, version TEXT)",
> What's this id used for?

Nothing.  But if you don't have one, SQLite will create one anyway and just
leave it unnamed.  So there's no reason not to.

> WebCore/storage/IDBObjectStoreBackendImpl.cpp:55
>  +	  loadIndexes();
> Can we do this lazily? It looks like m_indexes isn't needed until script
requests it?

No, because meta-data is accessed synchronously.
 
> WebCore/storage/IDBObjectStoreBackendImpl.cpp:75
>  +	      return "keyString IS NULL  AND  keyDate IS NULL  AND  keyNumber
IS NULL  AND  objectStoreId = ?";
> Do we need the NULL tests for the 'other' columns (except maybe the 'NULL'
type)? These should be NULL unless we have a logic error, right? If we want to
test for logic errors, we could do that in an ASSERT/debug statement?

Yeah, I guess that makes sense.
 
> WebCore/storage/IDBObjectStoreBackendImpl.cpp:66
>  +  static String whereClause(IDBKey::Type type)
> When I first read this method, I assumed the returned clause would include
'WHERE'. I think that makes more sense.

k

> WebCore/storage/IDBObjectStoreBackendImpl.cpp:106
>  +	  query.bindInt64(2, m_id);
> Could you use constants for these column indexes? Especially as they're
implementation details of whereClause().

I don't see a point to using constants.  They're tied to the SELECT statements
directly above.  Putting a constant at the beginning of the file makes no
sense.	Putting it next to the select seems only marginally better.

> WebCore/storage/IDBObjectStoreBackendImpl.cpp:207
>  +	  SQLiteStatement insert(sqliteDatabase(), "INSERT INTO Indexes (name,
keyPath, isUnique) VALUES (?, ?, ?)");
> This is missing the foreign key value

Indeed.


Will post another version soon.