Bug 27541 – Web Inspector: Split InspectorController into InspectorController and InspectorBackend

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

[Pavel Feldman]

There are a lot of methods in InspectorController, some are called from within front-end (by means of bindings), other form InspectorController's WebCore API. It would be better to have explicit InspectorBackend object bound to the JS that front-end talks to. InspectorController.idl will transform into InspectorBackend.idl, InspectorController's API will become much more clean. Before I start with the implementation, I'd like to hear your suggestions / thoughts.

[Timothy Hatcher]

I think this makes sense. I don't have any real suggestions.

[Patrick Mueller]

Sounds good.  I haven't quite figured out where the C++ code calls into front-end JS (haven't really looked, actually); or maybe it really doesn't at all or not much.  But it would be nice to have this information as well.

I have this dream of being able to take the current js/html/css/images out of front-end, and running them in a actual browser, plugging the back-end with something else.  

In addition to, or as part of that dream, folks seem to be starting to ask about remote debugging, and having a clear contract between the UI and the backend would be very helpful.

[Pavel Feldman]

(In reply to comment #2)
> Sounds good.  I haven't quite figured out where the C++ code calls into
> front-end JS (haven't really looked, actually); or maybe it really doesn't at
> all or not much.  But it would be nice to have this information as well.
> 

it is all going though the InspectorFrontend interface. See calls to m_frontend in InspectorController.

> I have this dream of being able to take the current js/html/css/images out of
> front-end, and running them in a actual browser, plugging the back-end with
> something else.  
> 

This is working with some limitations in Chromium. There is an InspectorController stub implementation written in JS code that handles most of the calls and makes front-end self-contained web app. Exceptions are calls to the host such as addSourceToFrame since they actually manipulate WebCore API. Here is a link: http://src.chromium.org/viewvc/chrome/trunk/src/webkit/glue/devtools/js/inspector_controller.js?revision=20751&view=markup

> In addition to, or as part of that dream, folks seem to be starting to ask
> about remote debugging, and having a clear contract between the UI and the
> backend would be very helpful.

Current plan is to serialize inspector controller frontend / backend interaction, so this should be possible. However, when talking about remote debugging, we should consider protocols such as http://my.opera.com/dragonfly/blog/scope-protocol-specification or similar.

[Pavel Feldman]

Created attachment 33333 [details]
Step 1

Step 1: 
- split classes, 
- move whatever is trivial to the InspectorBackend class
- hide private stuff in InspectorController
- delegate non-trivial calls such as highlighting-related.

[Timothy Hatcher]

Comment on attachment 33333 [details]
Step 1

What cleans up the plain pointer reference the InspectorBackend has to the InspectorController? It looks like you want to make InspectorController not be RefCounted anymore, but I don't see a change in the class inheritence.

Basically what guarentees the InspectorBackend has a valid InspectorController reference still when InspectorBackend is RefCounted and can be wrapped to JavaScript objects that have a GC lifetime?

Maybe InspectorController needs to stay RefCounted and still have RefPtr on Page or when InspectorController goes away it disconnects from InspectorBackend.

Otherwise patch looks great!

[Pavel Feldman]

(In reply to comment #5)
> (From update of attachment 33333 [details])
> What cleans up the plain pointer reference the InspectorBackend has to the
> InspectorController? It looks like you want to make InspectorController not be
> RefCounted anymore, but I don't see a change in the class inheritence.
> 
> Basically what guarentees the InspectorBackend has a valid InspectorController
> reference still when InspectorBackend is RefCounted and can be wrapped to
> JavaScript objects that have a GC lifetime?
> 
> Maybe InspectorController needs to stay RefCounted and still have RefPtr on
> Page or when InspectorController goes away it disconnects from
> InspectorBackend.
> 
> Otherwise patch looks great!

InspectorController was meant to stop being ref-counted. I fixed Page's header, but missed InspectorController's inheritance somehow.

Let me clear InspectorBackend::m_inspectorController in the InspectorController's destructor to initiate 'disconnect'. I will also add guards to the places in the code where controller is being accessed via the backend object. Sounds good?

[Timothy Hatcher]

(In reply to comment #6)
> InspectorController was meant to stop being ref-counted. I fixed Page's header,
> but missed InspectorController's inheritance somehow.
> 
> Let me clear InspectorBackend::m_inspectorController in the
> InspectorController's destructor to initiate 'disconnect'. I will also add
> guards to the places in the code where controller is being accessed via the
> backend object. Sounds good?

Yep.

[Pavel Feldman]

Created attachment 33408 [details]
Step 1

Now disconnecting backend from the controller upon controller destruction.

[Pavel Feldman]

Created attachment 33417 [details]
Step 1

Validated Chromium part (v8 bindings).

[Pavel Feldman]

Created attachment 33418 [details]
Step 1

[Timothy Hatcher]

Comment on attachment 33418 [details]
Step 1


> +    if (m_inspectorController)
> +        return m_inspectorController->resourceTrackingEnabled();
> +    else
> +        return false;

This (and a few othe places) should be written like:

> +    if (m_inspectorController)
> +        return m_inspectorController->resourceTrackingEnabled();
> +    return false;

[Pavel Feldman]

Thanks for review.

Please do not land this - I want to fix it first and use svn rename for renamed files.

[Pavel Feldman]

Sending        WebCore/ChangeLog
Sending        WebCore/DerivedSources.make
Sending        WebCore/GNUmakefile.am
Sending        WebCore/WebCore.gypi
Sending        WebCore/WebCore.pro
Sending        WebCore/WebCore.xcodeproj/project.pbxproj
Sending        WebCore/WebCoreSources.bkl
Adding         WebCore/bindings/js/JSInspectorBackendCustom.cpp
Deleting       WebCore/bindings/js/JSInspectorControllerCustom.cpp
Sending        WebCore/bindings/js/ScriptObject.cpp
Sending        WebCore/bindings/js/ScriptObject.h
Sending        WebCore/bindings/v8/DOMObjectsInclude.h
Sending        WebCore/bindings/v8/DerivedSourcesAllInOne.cpp
Sending        WebCore/bindings/v8/ScriptObject.cpp
Sending        WebCore/bindings/v8/ScriptObject.h
Sending        WebCore/bindings/v8/V8Index.cpp
Sending        WebCore/bindings/v8/V8Index.h
Sending        WebCore/bindings/v8/custom/V8CustomBinding.h
Adding         WebCore/bindings/v8/custom/V8InspectorBackendCustom.cpp
Deleting       WebCore/bindings/v8/custom/V8InspectorControllerCustom.cpp
Adding         WebCore/inspector/InspectorBackend.cpp
Adding         WebCore/inspector/InspectorBackend.h
Adding         WebCore/inspector/InspectorBackend.idl
Sending        WebCore/inspector/InspectorController.cpp
Sending        WebCore/inspector/InspectorController.h
Deleting       WebCore/inspector/InspectorController.idl
Sending        WebCore/inspector/front-end/Resource.js
Sending        WebCore/inspector/front-end/ScriptsPanel.js
Sending        WebCore/page/Page.cpp
Sending        WebCore/page/Page.h
Transmitting file data ...........................
Committed revision 46390.

[Pavel Feldman]

Created attachment 33489 [details]
patch

[Pavel Feldman]

Created attachment 33490 [details]
(clearing review request)

[Adam Barth]

Comment on attachment 33418 [details]
Step 1

Clearing review flag to remove from commit queue.