|Summary:||IndexedDB: Inspector should handle null, string, and array keyPaths|
|Product:||WebKit||Reporter:||Joshua Bell <jsbell>|
|Component:||Web Inspector (Deprecated)||Assignee:||Vsevolod Vlasov <vsevik>|
|Version:||528+ (Nightly build)|
|Bug Depends on:||84207, 85298|
Description Joshua Bell 2012-04-18 17:06:45 PDT
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.
Comment 1 Joshua Bell 2012-05-21 14:20:14 PDT
This can be implemented as soon as 85298 lands (a subset of 84207)
Comment 2 Joshua Bell 2012-05-22 12:32:13 PDT
Ready to go. A stub was landed with 85298 but should be replaced.
Comment 4 Joshua Bell 2012-06-08 08:46:46 PDT
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)
Comment 7 Joshua Bell 2012-06-08 10:54:14 PDT
(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
Comment 8 Vsevolod Vlasov 2012-06-08 10:56:35 PDT
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.
Comment 10 Vsevolod Vlasov 2012-06-08 11:00:23 PDT
(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.
Comment 11 Pavel Feldman 2012-06-09 03:50:52 PDT
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.