Bug 55057 – Web Inspector: refactor inspect() workflow so that it did not push dom nodes.

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

[Pavel Feldman]

I am working on getting rig of DOM agent pushes - everything should happen upon request. This patch changes the way we handle inspect: instead of pushing nodes, we are telling front-end that inspect element has been requested. It is then up to front-end to request dom nodes and focus them in the tree. I also made inspect() work in a generic manner, using same routines for nodes, databases, storages and potentially new elements.

[Pavel Feldman]

Created attachment 83494 [details]
Patch

[WebKit Review Bot]

Attachment 83494 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7982172

[Pavel Feldman]

Created attachment 83496 [details]
Patch

[Pavel Feldman]

Created attachment 83546 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 83546 [details]
Patch

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

> Source/WebCore/bindings/js/JSInjectedScriptHostCustom.cpp:174
> +#if ENABLE(DATABASE)

Should be 2 lines above?

> Source/WebCore/bindings/js/JSInjectedScriptHostCustom.cpp:186
> +#if ENABLE(DOM_STORAGE)

same here

> Source/WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:218
>      if (args.Length() < 1)

Why is this not under the #if guard below?

> Source/WebCore/inspector/InjectedScript.cpp:176
> +void InjectedScript::inspectNode(Node* node)

There are no nodes in the worker context, we will need to find a way to get rid of this method along with Frame.h dependency there.

> Source/WebCore/inspector/InjectedScript.cpp:182
> +        function.appendArgument(InjectedScriptHost::nodeAsScriptValue(mainWorldScriptState(frame), node));

You should give the state from the m_injectedScriptObject, shouldn't you?

> Source/WebCore/inspector/InjectedScriptSource.js:70
> +        if (arguments.length === 0)

What is this check for? If you need to check whether the object is valid you should check it explicitely.

> Source/WebCore/inspector/InjectedScriptSource.js:213
> +    releaseObject: function(objectId)

Probably we could switch from the object groups to this method?

> Source/WebCore/inspector/InspectorDatabaseResource.cpp:41
> +static long nextUnusedId = 1;

What's the reason for this change?

[Pavel Feldman]

Comment on attachment 83546 [details]
Patch

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

>> Source/WebCore/bindings/js/JSInjectedScriptHostCustom.cpp:174
>> +#if ENABLE(DATABASE)
> 
> Should be 2 lines above?

No, this is a bug fix since these methods are called from within Javascript unconditionally. I actually fixed callframes as well.

>> Source/WebCore/bindings/js/JSInjectedScriptHostCustom.cpp:186
>> +#if ENABLE(DOM_STORAGE)
> 
> same here

ditto

>> Source/WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:218
>>      if (args.Length() < 1)
> 
> Why is this not under the #if guard below?

ditto

>> Source/WebCore/inspector/InjectedScript.cpp:182
>> +        function.appendArgument(InjectedScriptHost::nodeAsScriptValue(mainWorldScriptState(frame), node));
> 
> You should give the state from the m_injectedScriptObject, shouldn't you?

Done.

>> Source/WebCore/inspector/InjectedScriptSource.js:70
>> +        if (arguments.length === 0)
> 
> What is this check for? If you need to check whether the object is valid you should check it explicitely.

This is correct - I should be able to inspect and dump undefined, null, etc. This check did not change.

>> Source/WebCore/inspector/InspectorDatabaseResource.cpp:41
>> +static long nextUnusedId = 1;
> 
> What's the reason for this change?

Consistency. All ids are 1-based longs.

[Pavel Feldman]

Committed r79556: <http://trac.webkit.org/changeset/79556>