Implement WebKit side of IDBFactory::getDatabaseNames

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

Implement WebKit side of IDBFactory::databaseNames

[Joshua Bell]

Created attachment 107249 [details]
Source changes

[Joshua Bell]

Created attachment 107257 [details]
Patch

[David Grogan]

Comment on attachment 107257 [details]
Patch

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.

[David Grogan]

Hans, can you take a look at this, especially the leveldb specific stuff?

[Hans Wennborg]

(In reply to comment #3)
> > 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?

I'm curious about that too.
 
> > 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.

I think this is fine. I'm not sure about the name though: encodeMaxKeyForOrigin(), since it's not really the max key for this origin, but rather the min key for the next one. Maybe you could just use encodeMinKeyForOrigin(origin + "\x01") where you use this?

> 
> > Source/WebKit/chromium/src/IDBFactoryBackendProxy.h:-47
> > -    PassRefPtr<WebCore::DOMStringList> databases(void) const;
> 
> Hans, do you know any of the backstory here?

Sorry, no. I think that function has been sitting there since before I started on this.

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

It's this guy: http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#widl-IDBFactory-cmp-int-any-first-any-second which we haven't implemented yet (crbug.com/92039).

[Hans Wennborg]

Comment on attachment 107257 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107257&action=review

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:104
> +#endif

Yeah, it would be nice if this code could be de-duplicated.

> Source/WebCore/storage/IDBLevelDBBackingStore.cpp:182
> +    }

I think this looks fine.

> Source/WebCore/storage/IDBSQLiteBackingStore.cpp:202
> +    ASSERT(foundNames.isEmpty());

maybe we should have this assert in the LevelDB code too?

[Joshua Bell]

Comment on attachment 107257 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107257&action=review

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

I had it removed, but had to put it back in when I realized I needed to call IDBLevelDBBackingStore::open(securityOrigin.get(), dataDir, maximumSize, fileIdentifier, this) / IDBSQLiteBackingStore::open(securityOrigin.get(), dataDir, maximumSize, fileIdentifier, this) in the case where the the backing store hasn't been added to m_backingStoreMap yet in IDBFactoryBackendImpl::databaseNames - I believe that's the case when the backing store has not yet been accessed for this origin during this session.

If that parameter can be removed from IDBLevelDBBackingStore::open / IDBSQLiteBackingStore::open (move the constant to those implementations) then this is easy. Otherwise, I'll need more guidance. We could add a BackingStore::openIfExists() that doesn't create if not present, etc.

>> 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?

IDBFactory::databaseNames() needed a ScriptExecutionContext. The two models I found are (1) annotate a method so that the method is passed a context for use within the method call, or (2) make the object an ActiveDOMObject which holds a handle to the document/SEC for the lifetime of the object. IDBFactory::open() was annotated in the IDL to take a SEC parameter, but IDBFactory::displayNames is an attribute in the IDL and those apparently can't be so annotated. I followed the ActiveDOMObject model used by some of the other IDB objects.

>> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:104
>> +#endif
> 
> Yeah, it would be nice if this code could be de-duplicated.

Absolutely! Wanted to ensure it was all correct/necessary before touching the rest of the code.

>> Source/WebCore/storage/IDBSQLiteBackingStore.cpp:202
>> +    ASSERT(foundNames.isEmpty());
> 
> maybe we should have this assert in the LevelDB code too?

Agreed, I can sprinkle those in. (That was a copy/paste from the SQLite getIndexes) - did the same for getIndexes and getObjectStores

>> LayoutTests/storage/indexeddb/key-type-negative-zero-expected.txt:1
>> +Test IndexedDB key types
> 
> Put the zero stuff in a separate bug.

Will do.

[Joshua Bell]

(In reply to comment #5)
> > > 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.
> 
> I think this is fine. I'm not sure about the name though: encodeMaxKeyForOrigin(), since it's not really the max key for this origin, but rather the min key for the next one. Maybe you could just use encodeMinKeyForOrigin(origin + "\x01") where you use this?

Possible. I was trying to follow the model of the other LevelDB iterators, which generate a start key using encode() then a stop key using encodeMaxKey() - which is typically the MAX_INT64 value. I wanted to hide the min/max logic in the implementation in IDBLevelDBCoding rather than having IDBLevelDBBackingStore be aware of the storage details.

I will change the implementation to that (more readable)... but how about encodeStopKeyForOrigin() as a name?

[Joshua Bell]

Created attachment 107358 [details]
Patch

[Joshua Bell]

Comment on attachment 107358 [details]
Patch

Trivial changes, mostly just testing the webkit-patch workflow.

[David Grogan]

Comment on attachment 107358 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107358&action=review

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:86
> +PassRefPtr<DOMStringList> IDBFactoryBackendImpl::databaseNames(PassRefPtr<SecurityOrigin> securityOrigin, Frame*, const String& dataDir, int64_t maximumSize, BackingStoreType backingStoreType)

Josh walked me through the code - it's easier to pass the quota around instead of taking it out.  The issue is that databaseNames opens the backing store and the backing store caches itself in m_backingStoreMap.  1Instead of monkeying with the caching logic, we can just take the quota out when we remove sqlite.

[Hans Wennborg]

(In reply to comment #8)
> > I think this is fine. I'm not sure about the name though: encodeMaxKeyForOrigin(), since it's not really the max key for this origin, but rather the min key for the next one. Maybe you could just use encodeMinKeyForOrigin(origin + "\x01") where you use this?
> 
> Possible. I was trying to follow the model of the other LevelDB iterators, which generate a start key using encode() then a stop key using encodeMaxKey() - which is typically the MAX_INT64 value. I wanted to hide the min/max logic in the implementation in IDBLevelDBCoding rather than having IDBLevelDBBackingStore be aware of the storage details.
> 
> I will change the implementation to that (more readable)... but how about encodeStopKeyForOrigin() as a name?

Yes, that sounds fine.

[Joshua Bell]

Created attachment 107510 [details]
Patch

[Joshua Bell]

Created attachment 107513 [details]
Patch

[Joshua Bell]

Latest patches still has duplicate code w/ IDBFactory::open. I will refactor once everything else looks correct.

Please review, and pay special attention to the result value in the callback, which I believe goes through this conversion on the way from browser to renderer: DOMStringList -> WebDOMStringList -> vector<string16> -> IPC serialization -> vector<string16> -> WebDOMStringList -> DOMStringList

[David Grogan]

Comment on attachment 107513 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107513&action=review

Looks good.

> Source/WebCore/bindings/v8/custom/V8IDBAnyCustom.cpp:73
> +    case IDBAny::DOMStringListType:

I don't understand the IDBAny/V8 stuff well enough to know if we have an alternative to this. Hans, do you know better?

> Source/WebCore/storage/IDBFactory.cpp:64
> +    if (!scriptExecutionContext()->isDocument()) {

If we do keep IDBFactory as an ActiveDOMObject, check !scriptExecutionContext() as well, it can go null from underneath us.

> Source/WebCore/storage/IDBFactory.cpp:81
> +    if (!scriptExecutionContext()->isDocument()) {

Same here.

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

Hans, do you know of disadvantages or costs to making something an ActiveDOMObject?

IDBFactory doesn't have to be an ActiveDOMObject now that databaseNames is async, but I asked Josh to leave it here so that I could ask this question :)  If you don't know we can ask japhet during the real review.

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:72
> +    // FIXME: ensure blank name is not problematic

After tracing this parameter through to 

http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/content_settings/tab_specific_content_settings.cc&l=269
and
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/browsing_data_indexed_db_helper.h&l=107

it looks like it is never used.  Maybe just change it to "Database Listing"?

> LayoutTests/storage/indexeddb/factory-basics.html:20
> +    indexedDB = evalAndLog("indexedDB = window.indexedDB || window.webkitIndexedDB || window.mozIndexedDB;");

Obiter dictum: in a separate CL we should probably update all of these to include msIndexedDB.

> LayoutTests/storage/indexeddb/factory-basics.html:24
> +    description = "My Test Database";

Could you add a call to getDatabaseNames before the open call and test that the result is empty?  It's more to ensure that we don't crash than for correctness.

[Joshua Bell]

Created attachment 107569 [details]
Patch

[Hans Wennborg]

(In reply to comment #16)
> > Source/WebCore/bindings/v8/custom/V8IDBAnyCustom.cpp:73
> > +    case IDBAny::DOMStringListType:
> 
> I don't understand the IDBAny/V8 stuff well enough to know if we have an alternative to this. Hans, do you know better?

I think this is fine. IIRC, so far we haven't returned a request result which is a DOMStringList, and that's why this is needed.

> > Source/WebCore/storage/IDBFactory.h:49
> > +class IDBFactory : public RefCounted<IDBFactory>, public ActiveDOMObject {
> 
> Hans, do you know of disadvantages or costs to making something an ActiveDOMObject?
> 
> IDBFactory doesn't have to be an ActiveDOMObject now that databaseNames is async, but I asked Josh to leave it here so that I could ask this question :)  If you don't know we can ask japhet during the real review.

Sorry, I have no idea.

[Michael Nordman]

> > > Source/WebCore/storage/IDBFactory.h:49
> > > +class IDBFactory : public RefCounted<IDBFactory>, public ActiveDOMObject {
> > 
> > Hans, do you know of disadvantages or costs to making something an ActiveDOMObject?
> > 
> > IDBFactory doesn't have to be an ActiveDOMObject now that databaseNames is async, but I asked Josh to leave it here so that I could ask this question :)  If you don't know we can ask japhet during the real review.

Since the factory hangs off of DOMWindow, I don't think ActiveDOMObject applies to this class.

ActiveDOMObject is mostly a means of keeping objects alive that have async behavior such that if the caller drops their js ref, the backing c++ impl doesn't disappear. But this doesn't apply to things that DOMWindow holds a reference to since the window will keep them alive for the duration.

        template<class T> void setPendingActivity(T* thisObject)
        {
            ASSERT(thisObject == this);
            thisObject->ref();   // <---- This keeps things alive
            m_pendingActivityCount++;
        }

        template<class T> void unsetPendingActivity(T* thisObject)
        {
            ASSERT(m_pendingActivityCount > 0);
            --m_pendingActivityCount;
            thisObject->deref();
        }

[Joshua Bell]

(In reply to comment #19)
> Since the factory hangs off of DOMWindow, I don't think ActiveDOMObject applies to this class.

Okay - I'll back that out. I initially added it when I was implementing this as a sync attribute which needed a ScriptExecutionContext and it wasn't apparent how to plumb that into an attribute call. Now that it's a method it's easy.

[Joshua Bell]

Created attachment 107926 [details]
Patch

[Michael Nordman]

Comment on attachment 107926 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107926&action=review

> Source/WebKit/chromium/src/WebIDBFactoryImpl.cpp:87
> +    if (dataDir.isEmpty() && backingStoreType == LevelDBBackingStore) {

This method looks identical to the open() method up to the point of calling thru to the m_idbFactoryBackend instance. Can that common code be shared?

[Joshua Bell]

Created attachment 108233 [details]
Patch

[Joshua Bell]

Latest patch is just the "part 1" of a two-sided patch. Will be followed by a Chromium patch - see http://codereview.chromium.org/7889024/ - and the rest of the WebKit implementation as a separate submission.

[Joshua Bell]

Comment on attachment 108233 [details]
Patch

Tony, can you review this

[Tony Chang]

Comment on attachment 108233 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108233&action=review

> Source/WebKit/chromium/public/WebIDBCallbacks.h:40
> +class WebDOMStringList;

Nit: alphabetize

[Joshua Bell]

Created attachment 108255 [details]
Patch

[WebKit Review Bot]

Comment on attachment 108255 [details]
Patch

Clearing flags on attachment: 108255

Committed r95698: <http://trac.webkit.org/changeset/95698>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.