Bug 27541 - Web Inspector: Split InspectorController into InspectorController and InspectorBackend
Summary: Web Inspector: Split InspectorController into InspectorController and Inspect...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-22 07:48 PDT by Pavel Feldman
Modified: 2009-09-11 20:05 PDT (History)
2 users (show)

See Also:


Attachments
Step 1 (101.84 KB, patch)
2009-07-23 08:52 PDT, Pavel Feldman
timothy: review-
Details | Formatted Diff | Diff
Step 1 (105.61 KB, patch)
2009-07-23 23:14 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
Step 1 (105.68 KB, text/plain)
2009-07-24 02:59 PDT, Pavel Feldman
no flags Details
Step 1 (105.68 KB, patch)
2009-07-24 03:00 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
patch (2.35 KB, patch)
2009-07-25 02:40 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
(clearing review request) (138 bytes, text/plain)
2009-07-25 03:00 PDT, Pavel Feldman
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2009-07-22 07:48:01 PDT
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.
Comment 1 Timothy Hatcher 2009-07-22 12:00:44 PDT
I think this makes sense. I don't have any real suggestions.
Comment 2 Patrick Mueller 2009-07-22 12:30:57 PDT
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.
Comment 3 Pavel Feldman 2009-07-22 14:04:11 PDT
(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.
Comment 4 Pavel Feldman 2009-07-23 08:52:02 PDT
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 5 Timothy Hatcher 2009-07-23 10:22:48 PDT
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!
Comment 6 Pavel Feldman 2009-07-23 10:45:01 PDT
(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?
Comment 7 Timothy Hatcher 2009-07-23 10:56:43 PDT
(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.
Comment 8 Pavel Feldman 2009-07-23 23:14:02 PDT
Created attachment 33408 [details]
Step 1

Now disconnecting backend from the controller upon controller destruction.
Comment 9 Pavel Feldman 2009-07-24 02:59:46 PDT
Created attachment 33417 [details]
Step 1

Validated Chromium part (v8 bindings).
Comment 10 Pavel Feldman 2009-07-24 03:00:12 PDT
Created attachment 33418 [details]
Step 1
Comment 11 Timothy Hatcher 2009-07-24 06:16:28 PDT
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;
Comment 12 Pavel Feldman 2009-07-24 07:18:57 PDT
Thanks for review.

Please do not land this - I want to fix it first and use svn rename for renamed files.
Comment 13 Pavel Feldman 2009-07-25 02:23:19 PDT
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.
Comment 14 Pavel Feldman 2009-07-25 02:40:53 PDT
Created attachment 33489 [details]
patch
Comment 15 Pavel Feldman 2009-07-25 03:00:29 PDT
Created attachment 33490 [details]
(clearing review request)
Comment 16 Adam Barth 2009-07-25 14:31:54 PDT
Comment on attachment 33418 [details]
Step 1

Clearing review flag to remove from commit queue.