Bug 41834 – Web Inspector: migrate InspectorDOMAgent to the generated version of InspectorFrontend interface.

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

[Ilya Tikhonovsky]

%SUBJ%
The next step to support Remote Debugging in WebKit.

[Ilya Tikhonovsky]

Created attachment 60838 [details]
[patch] initial version. Not for review.

[Ilya Tikhonovsky]

Created attachment 60856 [details]
[patch] second iteration

[Pavel Feldman]

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

WebCore/inspector/InspectorDOMAgent.cpp:236
 +          m_frontend->setDocument(InspectorObject::create());
This should be InspectorValue::null() really. Frontend will support it.


WebCore/inspector/InspectorDOMAgent.cpp:289
 +              m_frontend->childNodeInserted(parentId, prevId, value.release());
I think this only adds complexity. We should keep ownership and rely upon the sync nature of the call.

WebCore/inspector/InspectorDOMAgent.cpp:1105
 +          m_frontend->didGetStyleSheet(callId, InspectorObject::create());
null()

[Yury Semikhatsky]

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

WebCore/bindings/js/ScriptEventListener.cpp:116
 +      return ustringToString(jsFunction->toString(mainWorldScriptState(document->frame())));
You should use jsListener->isolatedWorld() instead.

WebCore/bindings/v8/ScriptEventListener.cpp:123
 +      ScriptState* scriptState = mainWorldScriptState(document->frame());
It should be possible to get it from the listener, something like 
static_cast<V8AbstractEventListener*>(listener)->worldContext()
worldContext is protected though.

[Ilya Tikhonovsky]

Created attachment 60908 [details]
[patch] third iteration.

ScriptEventListener's problem was fixed.
null is used instead of empty InspectorObject.

[Joseph Pecoraro]

Comment on attachment 60908 [details]
[patch] third iteration.

Sorry for the late review. Just minor comments, mostly about things
that were in the surrounding code, that you might want to incorporate
or think about in your next patch.

> +++ b/WebCore/inspector/InspectorController.h
> @@ -220,6 +220,7 @@ public:
>      InspectorFrontend* inspectorFrontend() { return m_frontend.get(); }
> +    InspectorFrontend2* inspectorFrontend2() { return m_frontend2.get(); }

I think both of these can be made "const".



> diff --git a/WebCore/inspector/InspectorDOMAgent.h b/WebCore/inspector/InspectorDOMAgent.h
>  #include "ScriptState.h"

Is this one still needed?


> @@ -57,7 +56,7 @@ namespace WebCore {
>      class ContainerNode;
>      ...
>      class NameNodeMap;
>      class Page;

I think these can all be removed. I think while you are making
some these changes it would be nice if headers and forward
declarations like this could be quickly checked. It would be a
great way to clean things up!

[Adam Barth]

Comment on attachment 60908 [details]
[patch] third iteration.

Looks like there are still some comments to address.

[Pavel Feldman]

I think this las landed long ago.