[webkit-reviews] review denied: [Bug 53169] Web Inspector: move InspectorController's methods from InspectorAgent to InspectorController : [Attachment 80192] [patch] initial version. for trybots

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Ilya Tikhonovsky
<loislo at chromium.org>'s request for review:
Bug 53169: Web Inspector: move InspectorController's methods from
InspectorAgent to InspectorController
https://bugs.webkit.org/show_bug.cgi?id=53169

Attachment 80192: [patch] initial version. for trybots
https://bugs.webkit.org/attachment.cgi?id=80192&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=80192&action=review

A bunch of nits, otherwise looks good.

> Source/WebCore/ChangeLog:12
> +	   https://bugs.webkit.org/show_bug.cgi?id=53169

ChangeLog structure should be:

Web Inspector: <Bug title>
http://bug
<blank line>
Description.
<blank line>

> Source/WebCore/inspector/InspectorController.cpp:167
> +    m_inspectorAgent->evaluateForTestInFrontend(callId, script);

This can go straight into the front-end.

> Source/WebCore/inspector/InspectorController.h:65
> +    InspectorAgent* inspectorAgent() const { return m_inspectorAgent.get();
}

You can't expose agent from inspector controller.

> Source/WebCore/inspector/InspectorController.h:66
> +    InspectorBackendDispatcher* inspectorBackendDispatcher() { return
m_inspectorBackendDispatcher.get(); }

replace with wrapper method: dispatchMessageFromFrontend

> Source/WebCore/inspector/InspectorController.h:67
> +    InspectorClient* inspectorClient() const { return m_inspectorClient; }

Should not be exposed. Or at least put a FIXME here.

> Source/WebCore/inspector/InspectorController.h:82
> +    void reuseFrontend();

I don't think we have reuseFrontend.

> Source/WebCore/inspector/InspectorController.h:87
> +    void highlight(Node*);

This code should move to HighlightUtilities class.

> Source/WebCore/inspector/InspectorController.h:94
> +    // FIXME: these methods are necessary for LayoutTestController but we
can do that with help of script injected into WebInspector page.

These should stay here.

> Source/WebCore/loader/FrameLoader.cpp:2981
> +    InspectorInstrumentation::resume(m_frame->page());

resume is not a part of the instrumentation, this is a control statement,
should be called on InspectorController.