Bug 78503 - Web Inspector: [InspectorIndexedDB] Pass data entries from object stores and indexes to front-end.
Summary: Web Inspector: [InspectorIndexedDB] Pass data entries from object stores and ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Vsevolod Vlasov
URL:
Keywords:
Depends on:
Blocks: 75386
  Show dependency treegraph
 
Reported: 2012-02-13 09:21 PST by Vsevolod Vlasov
Modified: 2012-02-15 04:19 PST (History)
17 users (show)

See Also:


Attachments
Patch (64.69 KB, patch)
2012-02-13 09:29 PST, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (65.17 KB, patch)
2012-02-13 09:59 PST, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (65.36 KB, patch)
2012-02-13 10:28 PST, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (66.09 KB, patch)
2012-02-14 06:26 PST, Vsevolod Vlasov
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (68.56 KB, patch)
2012-02-14 08:19 PST, Vsevolod Vlasov
yurys: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vsevolod Vlasov 2012-02-13 09:21:01 PST
Pass data entries from object stores and indexes to front-end.
Comment 1 Vsevolod Vlasov 2012-02-13 09:29:53 PST
Created attachment 126782 [details]
Patch
Comment 2 Philippe Normand 2012-02-13 09:39:01 PST
Comment on attachment 126782 [details]
Patch

Attachment 126782 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11510895
Comment 3 Vsevolod Vlasov 2012-02-13 09:59:25 PST
Created attachment 126787 [details]
Patch
Comment 4 Gustavo Noronha (kov) 2012-02-13 10:20:46 PST
Comment on attachment 126787 [details]
Patch

Attachment 126787 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11509820
Comment 5 Vsevolod Vlasov 2012-02-13 10:28:02 PST
Created attachment 126790 [details]
Patch
Comment 6 WebKit Review Bot 2012-02-13 11:38:59 PST
Comment on attachment 126790 [details]
Patch

Attachment 126790 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11506886

New failing tests:
http/tests/inspector/indexeddb/database-data.html
Comment 7 Yury Semikhatsky 2012-02-14 00:09:48 PST
Comment on attachment 126790 [details]
Patch

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

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1456
> +ScriptValue SerializedScriptValue::deserializeForInspector(ScriptState* scriptState)

We may guard it with #if ENABLE(INSPECTOR)

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1458
> +    JSValue value = deserialize(scriptState, scriptState->dynamicGlobalObject(), 0);

It might be safer to use lexicalGlobalObject() since it would work even if there is no js running in the script state.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:2263
> +    V8LocalContext localContext;

This will create a new context. You should take one from the ScriptState instead and enter it here instead. r- for this.

> Source/WebCore/inspector/Inspector.json:980
> +                    { "name": "type", "type": "number", "description": "Key type." },

This one should be enum.

> Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:333
> +        break;

We should report an error here if the type is invalid.

> Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:340
> +    RefPtr<InspectorObject> lower = keyRange->getObject("lower");

Could we reuse generated classes for parsing protocol messages? They seem to be already capable of validating InspectorValues and performing runtimeCast on them.
Comment 8 Vsevolod Vlasov 2012-02-14 06:26:46 PST
Created attachment 126969 [details]
Patch
Comment 9 WebKit Review Bot 2012-02-14 06:28:49 PST
Attachment 126969 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
From git://git.webkit.org/WebKit
   7073cfc..20e4c62  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 107708 = 7073cfc483f9201a0237ac9090c1ca6bb424de97
r107709 = 20e4c620dea55354276b040b3c141192647fc265
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.
        
<stdin>:1647: trailing whitespace.
    
<stdin>:1657: trailing whitespace.
    
<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.
    
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168776 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Pavel Feldman 2012-02-14 07:48:51 PST
Comment on attachment 126969 [details]
Patch

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

> Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:443
> +        RefPtr<InspectorObject> wrappedValue = m_injectedScript.wrapSerializedObject(value.get(), "indexeddb");

Could you use a constant for this? I don't see where you release the group. If you release objects on instance basis, you can leave this group name empty.

> Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:467
> +            m_frontendProvider->frontend()->objectStoreDataLoaded(m_requestId, m_result.release(), hasMore);

Provider implies that front-end field can be disposed. Should you check it here?

> Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:600
> +static Frame* assertFrame(const String& frameId, InspectorPageAgent* pageAgent, ErrorString* error)

ErrorString is usually the first parameter (as opposed to ExceptionCode)

> Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:610
> +static Document* assertDocument(Frame* frame, ErrorString* error)

ditto
Comment 11 Vsevolod Vlasov 2012-02-14 08:16:14 PST
(In reply to comment #7)
> (From update of attachment 126790 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126790&action=review
> 
> > Source/WebCore/bindings/js/SerializedScriptValue.cpp:1456
> > +ScriptValue SerializedScriptValue::deserializeForInspector(ScriptState* scriptState)
> 
> We may guard it with #if ENABLE(INSPECTOR)
Done.

> > Source/WebCore/bindings/js/SerializedScriptValue.cpp:1458
> > +    JSValue value = deserialize(scriptState, scriptState->dynamicGlobalObject(), 0);
> 
> It might be safer to use lexicalGlobalObject() since it would work even if there is no js running in the script state.
Done.

> > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:2263
> > +    V8LocalContext localContext;
> 
> This will create a new context. You should take one from the ScriptState instead and enter it here instead. r- for this.
Done.

> > Source/WebCore/inspector/Inspector.json:980
> > +                    { "name": "type", "type": "number", "description": "Key type." },
> 
> This one should be enum.
Done.

> > Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:333
> > +        break;
> 
> We should report an error here if the type is invalid.
Done.

> > Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:340
> > +    RefPtr<InspectorObject> lower = keyRange->getObject("lower");
> 
> Could we reuse generated classes for parsing protocol messages? They seem to be already capable of validating InspectorValues and performing runtimeCast on them.
Unfortunately we don't have typed getters yet.
Comment 12 Vsevolod Vlasov 2012-02-14 08:17:10 PST
(In reply to comment #10)
> (From update of attachment 126969 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126969&action=review
> 
> > Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:443
> > +        RefPtr<InspectorObject> wrappedValue = m_injectedScript.wrapSerializedObject(value.get(), "indexeddb");
> 
> Could you use a constant for this? I don't see where you release the group. If you release objects on instance basis, you can leave this group name empty.
Removed group name.

> > Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:467
> > +            m_frontendProvider->frontend()->objectStoreDataLoaded(m_requestId, m_result.release(), hasMore);
> 
> Provider implies that front-end field can be disposed. Should you check it here?
Thanks, I forgot this check in the beginning of this method.

> > Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:600
> > +static Frame* assertFrame(const String& frameId, InspectorPageAgent* pageAgent, ErrorString* error)
> 
> ErrorString is usually the first parameter (as opposed to ExceptionCode)
Done.

> > Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:610
> > +static Document* assertDocument(Frame* frame, ErrorString* error)
> 
> ditto
Done.
Comment 13 WebKit Review Bot 2012-02-14 08:18:42 PST
Comment on attachment 126969 [details]
Patch

Attachment 126969 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11514343

New failing tests:
http/tests/inspector/indexeddb/database-data.html
Comment 14 Vsevolod Vlasov 2012-02-14 08:19:26 PST
Created attachment 126981 [details]
Patch
Comment 15 WebKit Review Bot 2012-02-14 08:21:17 PST
Attachment 126981 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.
        
<stdin>:1647: trailing whitespace.
    
<stdin>:1657: trailing whitespace.
    
<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.
    
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168776 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 WebKit Review Bot 2012-02-14 09:23:37 PST
Comment on attachment 126981 [details]
Patch

Attachment 126981 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11523281

New failing tests:
http/tests/inspector/indexeddb/database-data.html
Comment 17 WebKit Review Bot 2012-02-14 10:12:18 PST
Comment on attachment 126981 [details]
Patch

Attachment 126981 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11518335

New failing tests:
http/tests/inspector/indexeddb/database-data.html
Comment 18 Yury Semikhatsky 2012-02-14 23:16:13 PST
Comment on attachment 126981 [details]
Patch

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

> Source/WebCore/inspector/InjectedScript.cpp:181
> +    ScriptValue scriptValue = serializedScriptValue->deserializeForInspector(m_injectedScriptObject.scriptState());

Can deserialization fail? If so, we should check for an empty scriptValue here to avoid crash.
Comment 19 Vsevolod Vlasov 2012-02-15 04:19:32 PST
Committed r107801: <http://trac.webkit.org/changeset/107801>