[webkit-reviews] review granted: [Bug 92558] IndexedDB: Core upgradeneeded logic : [Attachment 155914] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted David Grogan
<dgrogan at chromium.org>'s request for review:
Bug 92558: IndexedDB: Core upgradeneeded logic
https://bugs.webkit.org/show_bug.cgi?id=92558

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=155914&action=review


Just a bunch of style nits

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:325
> +	   // If this was an open-with-version call, there should be a "second

Make this a FIXME maybe?

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:447
> +    ASSERT_NOT_REACHED();

It's a little confusing to have this without an associated FIXME.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:458
> +    // The spec dictates we wait until all the version change events are

Should this be a FIXME?

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:470
> +    RefPtr<DOMStringList> objectStoreNames = DOMStringList::create();

Nit: I'd put a line-break before this line just for readability after the early
return.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:478
> +    if
(!transaction->scheduleTask(createCallbackTask(&IDBDatabaseBackendImpl::setIntV
ersionInternal, database, requestedVersion, callbacks, transaction),
> +				     
createCallbackTask(&IDBDatabaseBackendImpl::resetVersion, database,
m_version))) {

This might be a little easier to read if you created the callback tasks before
the if-statement.

> Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.cpp:168
> +	   databaseBackend = IDBDatabaseBackendImpl::create(name,
backingStore.get(), m_transactionCoordinator.get(), this, uniqueIdentifier);

Nit: I'd prefer a line-break before this line for readability.

> Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.cpp:177
> +    if (version == IDBDatabaseMetadata::NoIntVersion)

Nit: would prefer a line-break before this line as well.