Web Inspector: [InspectorIndexedDB] Pass data entries from object stores and indexes to front-end.

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

[Vsevolod Vlasov]

Pass data entries from object stores and indexes to front-end.

[Vsevolod Vlasov]

Created attachment 126782 [details]
Patch

[Philippe Normand]

Comment on attachment 126782 [details]
Patch

Attachment 126782 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11510895

[Vsevolod Vlasov]

Created attachment 126787 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 126787 [details]
Patch

Attachment 126787 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11509820

[Vsevolod Vlasov]

Created attachment 126790 [details]
Patch

[WebKit Review Bot]

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

[Yury Semikhatsky]

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.

[Vsevolod Vlasov]

Created attachment 126969 [details]
Patch

[WebKit Review Bot]

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.

[Pavel Feldman]

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

[Vsevolod Vlasov]

(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.

[Vsevolod Vlasov]

(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.

[WebKit Review Bot]

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

[Vsevolod Vlasov]

Created attachment 126981 [details]
Patch

[WebKit Review Bot]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Yury Semikhatsky]

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.

[Vsevolod Vlasov]

Committed r107801: <http://trac.webkit.org/changeset/107801>