[webkit-reviews] review requested: [Bug 53169] Web Inspector: move InspectorController's methods from InspectorAgent to InspectorController : [Attachment 80927] [patch] second version

[bugzilla-daemon at webkit.org]

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

Attachment 80927: [patch] second version
https://bugs.webkit.org/attachment.cgi?id=80927&action=review

------- Additional Comments from Ilya Tikhonovsky <loislo at chromium.org>
(In reply to comment #2)
> (From update of attachment 80192 [details])
> 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>

done.

> 
> > Source/WebCore/inspector/InspectorController.cpp:167
> > +	 m_inspectorAgent->evaluateForTestInFrontend(callId, script);
> 
> This can go straight into the front-end.

done.

> 
> > Source/WebCore/inspector/InspectorController.h:65
> > +	 InspectorAgent* inspectorAgent() const { return
m_inspectorAgent.get(); }
> 
> You can't expose agent from inspector controller.

done.

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

done.

> > 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.

done.

> 
> > Source/WebCore/inspector/InspectorController.h:87
> > +	 void highlight(Node*);
> 
> This code should move to HighlightUtilities class.

I'll do this in a separate patch.


> 
> > 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.

done.