Bug 41834 - Web Inspector: migrate InspectorDOMAgent to the generated version of InspectorFrontend interface.
Summary: Web Inspector: migrate InspectorDOMAgent to the generated version of Inspecto...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Ilya Tikhonovsky
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-07 22:29 PDT by Ilya Tikhonovsky
Modified: 2010-08-10 22:41 PDT (History)
8 users (show)

See Also:


Attachments
[patch] initial version. Not for review. (50.37 KB, patch)
2010-07-07 22:30 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
[patch] second iteration (69.47 KB, patch)
2010-07-08 02:38 PDT, Ilya Tikhonovsky
yurys: review-
Details | Formatted Diff | Diff
[patch] third iteration. (70.60 KB, patch)
2010-07-08 10:34 PDT, Ilya Tikhonovsky
pfeldman: review+
abarth: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ilya Tikhonovsky 2010-07-07 22:29:29 PDT
%SUBJ%
The next step to support Remote Debugging in WebKit.
Comment 1 Ilya Tikhonovsky 2010-07-07 22:30:24 PDT
Created attachment 60838 [details]
[patch] initial version. Not for review.
Comment 2 Ilya Tikhonovsky 2010-07-08 02:38:42 PDT
Created attachment 60856 [details]
[patch] second iteration
Comment 3 Pavel Feldman 2010-07-08 03:15:44 PDT
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()
Comment 4 Yury Semikhatsky 2010-07-08 03:17:09 PDT
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.
Comment 5 Ilya Tikhonovsky 2010-07-08 10:34:17 PDT
Created attachment 60908 [details]
[patch] third iteration.

ScriptEventListener's problem was fixed.
null is used instead of empty InspectorObject.
Comment 6 Joseph Pecoraro 2010-07-09 00:10:45 PDT
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!
Comment 7 Adam Barth 2010-08-10 22:21:11 PDT
Comment on attachment 60908 [details]
[patch] third iteration.

Looks like there are still some comments to address.
Comment 8 Pavel Feldman 2010-08-10 22:41:06 PDT
I think this las landed long ago.