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.
I think this makes sense. I don't have any real suggestions.
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.
(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.
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.
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!
(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?
(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.
Created attachment 33408 [details] Step 1 Now disconnecting backend from the controller upon controller destruction.
Created attachment 33417 [details] Step 1 Validated Chromium part (v8 bindings).
Created attachment 33418 [details] Step 1
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;
Thanks for review. Please do not land this - I want to fix it first and use svn rename for renamed files.
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.
Created attachment 33489 [details] patch
Created attachment 33490 [details] (clearing review request)
Comment on attachment 33418 [details] Step 1 Clearing review flag to remove from commit queue.