Bug 113398 – Web Inspector: add event for node inspection request while in inspection mode

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Web Inspector (Deprecated)

[Dmitry Gozman]

Web Inspector: dispatch inspectNode to frontend without injected script. This will allow to inspect nodes while debugging.

[Dmitry Gozman]

Created attachment 195293 [details]
Patch

[Pavel Feldman]

Comment on attachment 195293 [details]
Patch

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

> Source/WebCore/inspector/Inspector.json:1992
> +                "name": "shouldInspectNode",

You don't want them on DOM domain, you also want them to be hidden.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1031
> +    int nodeId = pushNodePathToFrontend(m_nodeToFocus.get());

It might be too early to push the node - document might not be requested yet.

[Timothy Hatcher]

This seems overly complex — 2 methods and 1 event.

What problem does this solve? Why is this needed? "Why" should always be answered in the bug and/or change log.

[Dmitry Gozman]

(In reply to comment #3)
> This seems overly complex — 2 methods and 1 event.
> 
> What problem does this solve? Why is this needed? "Why" should always be answered in the bug and/or change log.

As written in the bug description, this will allow to inspect nodes while debugging. Otherwise, security checks disallow to evaluate anything in injected script, if debugger stopped in another context (i.e. in iframe).

As for 2 additional methods, ChangeLog contains the explanation.

[Dmitry Gozman]

Comment on attachment 195293 [details]
Patch

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

>> Source/WebCore/inspector/Inspector.json:1992
>> +                "name": "shouldInspectNode",
> 
> You don't want them on DOM domain, you also want them to be hidden.

Which domain then? Inspector? Is it OK to use them from InspectorDOMAgent class then (I mean, it will introduce dependence InspectorDOMAgent <-- InspectorFrontend::Inspector) ?

>> Source/WebCore/inspector/InspectorDOMAgent.cpp:1031
>> +    int nodeId = pushNodePathToFrontend(m_nodeToFocus.get());
> 
> It might be too early to push the node - document might not be requested yet.

If so, this method will not do anything, and frontend will request nodeToFocus via getInspectedNode instead (which will happen after pushing document). See explanation in ChangeLog.

[Timothy Hatcher]

Sorry I missed the bug description. Reading through it all again it makes sense, though still more complex than I would like.

[Dmitry Gozman]

Created attachment 195969 [details]
Patch

[Dmitry Gozman]

Created attachment 196109 [details]
Patch

[Dmitry Gozman]

Separate event in DOM domain is the right thing here.
This also solves the problem with inspecting in debug mode, since we don't go into injected script.

[Pavel Feldman]

Comment on attachment 196109 [details]
Patch

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

> Source/WebCore/inspector/Inspector.json:2074
> +                "name": "nodeShouldBeInspected",

inspectNodeRequested

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1033
> +    if (m_searchingForNode) {

Lets remove this "focusNode" method - it is misleading.

> Source/WebCore/inspector/front-end/DOMAgent.js:1050
> +        WebInspector.updateFocusedNode(nodeId);

DOMAgent is SDK, WebInspector is UI, no dependency is allowed.

[Dmitry Gozman]

Created attachment 196145 [details]
Patch

[Dmitry Gozman]

Comment on attachment 196109 [details]
Patch

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

>> Source/WebCore/inspector/Inspector.json:2074
>> +                "name": "nodeShouldBeInspected",
> 
> inspectNodeRequested

Done.

>> Source/WebCore/inspector/InspectorDOMAgent.cpp:1033
>> +    if (m_searchingForNode) {
> 
> Lets remove this "focusNode" method - it is misleading.

Done. Code separated between InspectorController::inspect and InspectorDOMAgent::inspect.

>> Source/WebCore/inspector/front-end/DOMAgent.js:1050
>> +        WebInspector.updateFocusedNode(nodeId);
> 
> DOMAgent is SDK, WebInspector is UI, no dependency is allowed.

Fixed.

[Pavel Feldman]

Comment on attachment 196145 [details]
Patch

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

> Source/WebCore/inspector/InspectorController.cpp:346
> +    InjectedScript injectedScript = m_injectedScriptManager->injectedScriptFor(mainWorldScriptState(frame));

if (node->nodeType() != Node::ELEMENT_NODE && node->nodeType() != Node::DOCUMENT_NODE) is now missing here.

[Dmitry Gozman]

Comment on attachment 196145 [details]
Patch

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

>> Source/WebCore/inspector/InspectorController.cpp:346
>> +    InjectedScript injectedScript = m_injectedScriptManager->injectedScriptFor(mainWorldScriptState(frame));
> 
> if (node->nodeType() != Node::ELEMENT_NODE && node->nodeType() != Node::DOCUMENT_NODE) is now missing here.

Nice catch! Thanks. Fixed.

[Dmitry Gozman]

Created attachment 196325 [details]
Patch