[webkit-reviews] review canceled: [Bug 68037] Implement WebKit side of IDBFactory::databaseNames : [Attachment 107257] Patch

[bugzilla-daemon at webkit.org]

David Grogan <dgrogan at chromium.org> has canceled Joshua Bell
<jsbell at chromium.org>'s request for review:
Bug 68037: Implement WebKit side of IDBFactory::databaseNames
https://bugs.webkit.org/show_bug.cgi?id=68037

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

------- Additional Comments from David Grogan <dgrogan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=107257&action=review


It looks good.	You didn't miss the mark anywhere, just some small stuff.  The
biggest is probably sync v async, we'll talk about it tomorrow.

> ChangeLog:8
> +	   * ../../LayoutTests/storage/indexeddb/factory-basics-expected.txt:
Added.

We'll have to figure out what's going on with the ../../

> Source/WebCore/storage/IDBFactory.cpp:75
> +    return m_factoryBackend->databaseNames(document->securityOrigin(),
document->frame(), groupSettings->indexedDBDatabasePath(),
groupSettings->indexedDBQuotaBytes(),
IDBFactoryBackendInterface::DefaultBackingStore);

Remove the quota parameter from here and all the other databaseNames function
signatures.

> Source/WebCore/storage/IDBFactory.h:49
> +class IDBFactory : public RefCounted<IDBFactory>, public ActiveDOMObject {

I'm not 100% sure that this won't cause problems but we'll see.  Actually, what
prompted you to make this an ActiveDOMObject?

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:103
> +	       backingStore =
IDBLevelDBBackingStore::open(securityOrigin.get(), dataDir, maximumSize,
fileIdentifier, this);

The code from open can probably be factored out to avoid the duplication.

> Source/WebCore/storage/IDBLevelDBCoding.cpp:810
> +    end.append("\x01"); // just after origin in collation order

I see what you mean about the sketchiness. Hans might know a better way.

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.h:-47
> -    PassRefPtr<WebCore::DOMStringList> databases(void) const;

Hans, do you know any of the backstory here?

> Source/WebKit/chromium/src/WebIDBFactoryImpl.cpp:89
> +	       // Layout tests provide a temporary folder.

Same comment as elsewhere about reusing the code from open.  Basically: do it
:)

> LayoutTests/storage/indexeddb/factory-basics.html:35
> +    // TODO: test indexedDB.deleteDatabase(databaseName)

Change this TODO to FIXME.  (In chromium we do 'TODO(username):', in webkit
just 'FIXME:')

> LayoutTests/storage/indexeddb/factory-basics.html:36
> +    // TODO: test indexedDB.cmp(key1, key2)

What's cmp?

> LayoutTests/storage/indexeddb/key-type-negative-zero-expected.txt:1
> +Test IndexedDB key types

Put the zero stuff in a separate bug.