Bug 42171 – Web Inspector: Clean-up InspectorBackend code.

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

[Ilya Tikhonovsky]

In the next changes InspectorBackend content will be generated by scripts.
As far as generator is very simple thing all nontrivial function should be 
moved to InspectorController and DOMAgent.

[Ilya Tikhonovsky]

Created attachment 61385 [details]
[patch] initial version.

[Ilya Tikhonovsky]

Created attachment 61483 [details]
[patch] second iteration

[Joseph Pecoraro]

Comment on attachment 61483 [details]
[patch] second iteration

My usual leftover comments =).

> -        [custorResponse=didApplyDomChange] void setAttribute(in long elementId, in String name, in String value, out boolean success);
> +        [customResponse=didApplyDomChange] void setAttribute(in long elementId, in String name, in String value, out boolean success);
> -#if defined(ENABLE_OFFLINE_WEB_APPLICATIONS)
> +#if defined(ENABLE_OFFLINE_WEB_APPLICATIONS) && ENABLE_OFFLINE_WEB_APPLICATIONS

Thanks!


> +++ b/WebCore/inspector/InspectorBackend.cpp
> -    RefPtr<InspectorResource> resource = m_inspectorController->resources().get(identifier);
> -    String markup = createMarkup(node);

Since InspectorBackend is now going to be generated it will be great
to remove all of the #includes that are no longer needed.


> +++ b/WebCore/inspector/InspectorController.cpp
> +void InspectorController::getResourceContent(long callId, unsigned long identifier)
> +{
> + ...
> +    else
> +        m_frontend->didGetResourceContent(callId, "");
> +}

I know this is just moving code, but I think the "" should be changed
to String() rather than taking the implicit cast. I think that is
the approach we take in other places.


> +++ b/WebCore/inspector/InspectorController.h
> +    void disableMonitoringXHR() {setMonitoringXHR(false); }

Missing a space after the opening brace.


>      InspectorDOMAgent* domAgent() { return m_domAgent.get(); }

Not changed code but could probably use const. "domAgent() const {"

[Pavel Feldman]

I think this is a really bad change and it should be rolled out. I've spent so much time trying to limit the InspectorController's responsibilities and drag as much as I could out of it. I wanted InspectorBackend to handle debugging and such as much as possible on its own. The idea was that backend handles all the requests and uses InspectorController only when it needs contorller's state. That way it could talk directly to DOM agent, future Resource agent and such. You make it all route to InspectorController again and it is again hard to tell which parts of IC's API are for WebCore clients and which are for the front-end.

I don't understand why this change is necessary for the remote debugging. All you need is to generate dispatch against InspectorBackend.h and since IDL binding works, generated dispatch should also work.

[Yury Semikhatsky]

(In reply to comment #4)
> I think this is a really bad change and it should be rolled out. I've spent so much time trying to limit the InspectorController's responsibilities and drag as much as I could out of it. I wanted InspectorBackend to handle debugging and such as much as possible on its own. The idea was that backend handles all the requests and uses InspectorController only when it needs contorller's state. That way it could talk directly to DOM agent, future Resource agent and such. You make it all route to InspectorController again and it is again hard to tell which parts of IC's API are for WebCore clients and which are for the front-end.
> 
AFAIR InspectorBackend were meant to define an interface exposed to the Web Inspector front end and nothing but this. Breaking InspectorController down into smaller pieces with limited responsibility(DOM/Resource/Debugger/Storage.. agents) is another problem. Now we have some debugger methods handled by InspectorBackend itself while others are implemented on InspectorController. What we need here is to extract debugger functionality into its own class. The only debugger method that was moved by Ilya is setPauseOnExceptionsState. I think it's better to move it to IC instead of providing custom parameter conversion for this method in generated code.  Another change that may be considered bad is moving getResourceContent to IC. But I would argue that having this method implemented on IB doesn't solve the problem of resource handling concentrated in IC. If we want to resolve this issue we should go ahead and extract resource managing into a separate class. 

> I don't understand why this change is necessary for the remote debugging. All you need is to generate dispatch against InspectorBackend.h and since IDL binding works, generated dispatch should also work.
If we are going to generate request dispatcher from Inspector.idl we need to get rid of custom logic in  InspectorBackend.cpp and make it delegate all requests to appropriate agents. To avoid blowing up InspectorController we should probably start extracting Resource/Debugger/Storage/.. agents first.

[Pavel Feldman]

As we discussed offline, InspectorBackend is a server implementation, it should not be generated. What we should generate instead is a dispatch code that converts message strings to calls against its instance.