Bug 129162 – Web Inspector: add user interface for IndexedDB

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Timothy Hatcher]

Hook up IndexedDB inspection.

[Radar WebKit Bug Importer]

<rdar://problem/16136778>

[Timothy Hatcher]

Created attachment 224923 [details]
Patch

[Build Bot]

Comment on attachment 224923 [details]
Patch

Attachment 224923 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5784519332331520

New failing tests:
inspector-protocol/storage/indexed-database.html

[Build Bot]

Created attachment 224927 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Joseph Pecoraro]

Comment on attachment 224923 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/IndexedDatabaseObjectStore.js:27
> +WebInspector.IndexedDatabaseObjectStore = function(name, keyPath, autoIncrement, indexes)
> +{

Nit: Missing super constructor call. WebInspector.Object.call(this)

> Source/WebInspectorUI/UserInterface/IndexedDatabaseObjectStore.js:61
> +    get autoIncrement()
> +    {
> +        return this._autoIncrement;
> +    },

Should we show something in the UI if this is an auto incrementing Object Store? It seems unused right now.

> Source/WebInspectorUI/UserInterface/IndexedDatabaseObjectStoreContentView.css:62
> + /* FIXME: This should show a alternating stripe, but I couldn't make it work. */

Typo: "a alternating" => "an alternating"

> Source/WebInspectorUI/UserInterface/IndexedDatabaseObjectStoreContentView.js:113
> +        for (var entry of this._entries) {
> +            entry.primaryKey.release();
> +            entry.key.release();
> +            entry.value.release();
> +        }

Arg, this looks horrible to me.

If there are 100 rows, this means we would need to send 300 messages to the backend to clear all these RemoteObjects? We should put these in a RemoteObject group and just send one message to clear the group. More on that below.

I don't think this blocks landing this patch, but it would be a good follow-up.

> Source/WebInspectorUI/UserInterface/IndexedDatabaseObjectStoreIndex.js:63
> +    get unique()
> +    {
> +        return this._unique;
> +    },
> +
> +    get multiEntry()
> +    {
> +        return this._multiEntry;
> +    },

Should we show these in the UI somehow?

> Source/WebInspectorUI/UserInterface/IndexedDatabaseObjectStoreIndexTreeElement.js:2
> + * Copyright (C) 2013 Apple Inc. All rights reserved.

Nit: 2014

> Source/WebInspectorUI/UserInterface/IndexedDatabaseObjectStoreTreeElement.js:2
> + * Copyright (C) 2013 Apple Inc. All rights reserved.

Nit: 2014

> Source/WebInspectorUI/UserInterface/StorageManager.js:36
> +    if (window.IndexedDBAgent)
> +        IndexedDBAgent.enable();

I think this will be mysterious on iOS 6 and iOS 7. Though this is in the protocol for iOS 6 and iOS 7, the feature wasn't enabled. I think we should remove the "IndexedDB" domain from the iOS 6 and iOS 7 versions files so that feature detection works as expected there:

    Source/WebInspectorUI/Versions/Inspector-iOS-6.0.json
    Source/WebInspectorUI/Versions/Inspector-iOS-7.0.json

> Source/WebInspectorUI/UserInterface/StorageManager.js:168
> +        IndexedDBAgent.requestData.invoke(requestArguments, processData.bind(this));

Nit: The .bind(this) is not needed. "this" is not use din the "processData".

We could be passing a "group id" string from here to the backend so we have a group to clear the resulting RemoteObjects all at once.

This eventually gets down to the following code in class OpenCursorCallback in InspectorIndexedDBAgent.cpp:

        RefPtr<DataEntry> dataEntry = DataEntry::create()
            .setKey(m_injectedScript.wrapObject(idbCursor->key(), String()))
            .setPrimaryKey(m_injectedScript.wrapObject(idbCursor->primaryKey(), String()))
            .setValue(m_injectedScript.wrapObject(idbCursor->value(), String()));

If instead of String() if we had provided a group name, we could use that!

> Source/WebInspectorUI/UserInterface/StorageManager.js:249
> +            var objectStores = databasePayload.objectStores.map(processObjectStore.bind(this));

A bunch of the .bind(this)s are not needed here. But I'll leave removing them up to you.

> LayoutTests/inspector-protocol/storage/indexed-database.html:1
> +<!DOCTYPE html>

I'd prefer the file path specify the domain. We've been going with that kind of hierarchy where possible, but maybe it is time to abandon it:

     LayoutTests/inspector-protocol/<domain>/<command>.html
     LayoutTests/inspector-protocol/indexeddb/something.html

[Timothy Hatcher]

Comment on attachment 224923 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/IndexedDatabaseObjectStore.js:61
>> +    },
> 
> Should we show something in the UI if this is an auto incrementing Object Store? It seems unused right now.

We could in the future. Maybe a details sidebar?

>> Source/WebInspectorUI/UserInterface/IndexedDatabaseObjectStoreContentView.js:113
>> +        }
> 
> Arg, this looks horrible to me.
> 
> If there are 100 rows, this means we would need to send 300 messages to the backend to clear all these RemoteObjects? We should put these in a RemoteObject group and just send one message to clear the group. More on that below.
> 
> I don't think this blocks landing this patch, but it would be a good follow-up.

Yeah, I was surprised by this too. I filed bug 129203.

>> Source/WebInspectorUI/UserInterface/IndexedDatabaseObjectStoreIndex.js:63
>> +    },
> 
> Should we show these in the UI somehow?

Maybe a details sidebar too?

>> Source/WebInspectorUI/UserInterface/StorageManager.js:36
>> +        IndexedDBAgent.enable();
> 
> I think this will be mysterious on iOS 6 and iOS 7. Though this is in the protocol for iOS 6 and iOS 7, the feature wasn't enabled. I think we should remove the "IndexedDB" domain from the iOS 6 and iOS 7 versions files so that feature detection works as expected there:
> 
>     Source/WebInspectorUI/Versions/Inspector-iOS-6.0.json
>     Source/WebInspectorUI/Versions/Inspector-iOS-7.0.json

Good idea. Fixed.

>> LayoutTests/inspector-protocol/storage/indexed-database.html:1
>> +<!DOCTYPE html>
> 
> I'd prefer the file path specify the domain. We've been going with that kind of hierarchy where possible, but maybe it is time to abandon it:
> 
>      LayoutTests/inspector-protocol/<domain>/<command>.html
>      LayoutTests/inspector-protocol/indexeddb/something.html

Done.

[Timothy Hatcher]

https://trac.webkit.org/changeset/164541