[Webkit-unassigned] [Bug 68037] Implement WebKit side of IDBFactory::databaseNames

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=68037





--- Comment #16 from David Grogan <dgrogan at chromium.org>  2011-09-15 14:54:51 PST ---
(From update of attachment 107513)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.