[webkit-reviews] review granted: [Bug 129162] Web Inspector: add user interface for IndexedDB : [Attachment 224923] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Timothy Hatcher
<timothy at apple.com>'s request for review:
Bug 129162: Web Inspector: add user interface for IndexedDB
https://bugs.webkit.org/show_bug.cgi?id=129162

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

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
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:6
2
> + /* FIXME: This should show a alternating stripe, but I couldn't make it
work. */

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

>
Source/WebInspectorUI/UserInterface/IndexedDatabaseObjectStoreContentView.js:11
3
> +	   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