[webkit-reviews] review granted: [Bug 125258] DatabaseProcess IndexedDB: Establish a metadata backing store on disk : [Attachment 219099] Patch v6 - Move database directory creation back into the database process.

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has granted Brady Eidson
<beidson at apple.com>'s request for review:
Bug 125258: DatabaseProcess IndexedDB: Establish a metadata backing store on
disk
https://bugs.webkit.org/show_bug.cgi?id=125258

Attachment 219099: Patch v6 - Move database directory creation back into the
database process.
https://bugs.webkit.org/attachment.cgi?id=219099&action=review

------- Additional Comments from Anders Carlsson <andersca at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=219099&action=review


> Source/WebCore/Modules/indexeddb/IDBDatabaseMetadata.cpp:45
> +    ObjectStoreMap::const_iterator i = objectStores.begin();
> +    ObjectStoreMap::const_iterator end = objectStores.end();
> +    for (; i != end; ++i)
> +	   result.objectStores.set(i->key, i->value.isolatedCopy());

Please move the declarations inside the loop:

for (auto it = objectStores.begin(), end = objectStores.end(); it != end; ++it)


> Source/WebCore/Modules/indexeddb/IDBDatabaseMetadata.cpp:63
> +    IndexMap::const_iterator i = indexes.begin();
> +    IndexMap::const_iterator end = indexes.end();
> +    for (; i != end; ++i)
> +	   result.indexes.set(i->key, i->value.isolatedCopy());

Same thing here.

> Source/WebCore/Modules/indexeddb/IDBKeyPath.cpp:257
> +    result.m_array.reserveCapacity(m_array.size());

This should use reserveInitialCapacity.

> Source/WebCore/Modules/indexeddb/IDBKeyPath.cpp:259
> +	   result.m_array.append(m_array[i].isolatedCopy());

This should use uncheckedAppend.

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:80
> +    ASSERT(!m_identifier.databaseName().isNull());
> +
> +    if (m_identifier.databaseName().isEmpty())
> +	   return "%00";
> +
> +    String filename = encodeForFileName(m_identifier.databaseName());
> +    if (filename == ".")
> +	   return "%2E";
> +
> +    if (filename == "..")
> +	   return "%2E%2E";
> +
> +    return filename;

Escape all the dots (Says Tim).

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:255
> +	   ref();

I think you should use a protection variable here instead of explicitly calling
ref()/deref().

>
Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQ
Lite.cpp:147
> +std::unique_ptr<WebCore::SQLiteDatabase>
UniqueIDBDatabaseBackingStoreSQLite::openSQLiteDatabaseAtPath(const String&
path)

No need for WebCore:: here.

> Source/WebKit2/Shared/AsyncTask.h:48
> +    AsyncTaskImpl(T* const callee, void (T::*method)(Arguments...),
Arguments&&... arguments)

I think callee should be a reference here (and everywhere else).

> Source/WebKit2/Shared/SecurityOriginData.h:53
> +    // Returns a string that is appropriate for use as a unique filename
> +    String databaseFilenameIdentifier() const;
> +
> +    // Returns true if this origin can use the same databases as the given
origin.
> +    bool canShareDatabases(const SecurityOriginData&) const;

We think these should be moved to the IDB code.