Bug 84303 - IndexedDB: Inspector should handle null, string, and array keyPaths
Summary: IndexedDB: Inspector should handle null, string, and array keyPaths
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Vsevolod Vlasov
URL:
Keywords:
Depends on: 84207 85298
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-18 17:06 PDT by Joshua Bell
Modified: 2012-06-09 05:01 PDT (History)
1 user (show)

See Also:


Attachments
Patch (20.47 KB, patch)
2012-06-08 08:07 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Screenshot1 (55.00 KB, image/png)
2012-06-08 10:49 PDT, Vsevolod Vlasov
no flags Details
Screenshot2 (90.88 KB, image/png)
2012-06-08 10:50 PDT, Vsevolod Vlasov
no flags Details
Patch (22.64 KB, patch)
2012-06-08 10:59 PDT, Vsevolod Vlasov
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 3 Vsevolod Vlasov 2012-06-08 08:07:22 PDT
Created attachment 146575 [details]
Patch
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 5 Vsevolod Vlasov 2012-06-08 10:49:50 PDT
Created attachment 146605 [details]
Screenshot1
Comment 6 Vsevolod Vlasov 2012-06-08 10:50:04 PDT
Created attachment 146606 [details]
Screenshot2
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 9 Vsevolod Vlasov 2012-06-08 10:59:03 PDT
Created attachment 146607 [details]
Patch
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.
Comment 12 Vsevolod Vlasov 2012-06-09 05:01:34 PDT
Committed r119901: <http://trac.webkit.org/changeset/119901>