Bug 84303

Summary: IndexedDB: Inspector should handle null, string, and array keyPaths
Product: WebKit Reporter: Joshua Bell <jsbell>
Component: Web Inspector (Deprecated)Assignee: Vsevolod Vlasov <vsevik>
Status: RESOLVED FIXED    
Severity: Normal CC: vsevik
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 84207, 85298    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Screenshot1
none
Screenshot2
none
Patch pfeldman: review+

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>