Bug 84303 – IndexedDB: Inspector should handle null, string, and array keyPaths

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Joshua Bell]

When the IDB inspector was implemented only string-type key paths were supported for the IDBObjectStore.keyPath and IDBIndex.keyPath properties. Null was not distinguished from the empty string.

https://bugs.webkit.org/show_bug.cgi?id=84207 is adding support for array-type keypaths, which will need to be displayed somehow. Also, null and empty string should be distinguished, as the behavior differs.

[Joshua Bell]

This can be implemented as soon as 85298 lands (a subset of 84207)

[Joshua Bell]

Ready to go. A stub was landed with 85298 but should be replaced.

[Vsevolod Vlasov]

Created attachment 146575 [details]
Patch

[Joshua Bell]

Comment on attachment 146575 [details]
Patch

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

overall lgtm

> Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:244
> +        Vector<String> stringArray = idbKeyPath.array();

Can you make this const Vector<String>& stringArray to avoid a copy?

> Source/WebCore/inspector/front-end/IndexedDBModel.js:158
> +        return "\"" + idbKeyPath + "\"";

Do we care about escaping strings that contain " characters (etc)? E.g. using JSON.stringify(s) or something?

(I don't think I've seen this in the inspector myself, so as long as we're consistent)

[Vsevolod Vlasov]

Created attachment 146605 [details]
Screenshot1

[Vsevolod Vlasov]

Created attachment 146606 [details]
Screenshot2

[Joshua Bell]

(In reply to comment #5)
> Created an attachment (id=146605) [details]
> Screenshot1

Nice - I notice that the closing ) is missing or truncated in the column headers in the screenshots, i.e.:

Key (keyPath: "string"
Primary key (keyPath: "id.index"
Key (keyPath: ["number", "string"]

Just a display issue? It looks like it's present in the code in _keyPathHeader

[Vsevolod Vlasov]

You caught me in the middle of uploading a new patch :)
I've already spotted the closing bracket problem and a couple of others.
I am going to fix these and upload a new version soon.

[Vsevolod Vlasov]

Created attachment 146607 [details]
Patch

[Vsevolod Vlasov]

(In reply to comment #4)
> (From update of attachment 146575 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146575&action=review
> 
> overall lgtm
> 
> > Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:244
> > +        Vector<String> stringArray = idbKeyPath.array();
> 
> Can you make this const Vector<String>& stringArray to avoid a copy?
Done.

> > Source/WebCore/inspector/front-end/IndexedDBModel.js:158
> > +        return "\"" + idbKeyPath + "\"";
> 
> Do we care about escaping strings that contain " characters (etc)? E.g. using JSON.stringify(s) or something?
> 
> (I don't think I've seen this in the inspector myself, so as long as we're consistent)
keyPathString is now used only for tooltips and keyPath itself comes from the backend so I don't think we really need escaping here.

[Pavel Feldman]

Comment on attachment 146607 [details]
Patch

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

> Source/WebCore/inspector/front-end/IndexedDBViews.js:170
> +        keyColumnHeaderFragment.appendChild(document.createTextNode(" (" + WebInspector.UIString("keyPath") + ": "));

Do you need to localize "keyPath"? If yes, it should be UIString("(keyPath: "), etc.

[Vsevolod Vlasov]

Committed r119901: <http://trac.webkit.org/changeset/119901>