Bug 52472 – Web Inspector: simplify debugger enabling routine.

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

[Pavel Feldman]

Patch to follow.

[Pavel Feldman]

Created attachment 78989 [details]
[PATCH] Proposed change

[Ilya Tikhonovsky]

Comment on attachment 78989 [details]
[PATCH] Proposed change

View in context: https://bugs.webkit.org/attachment.cgi?id=78989&action=review

> WebKit2/WebProcess/WebPage/WebInspector.cpp:104
>  void WebInspector::startJavaScriptDebugging()
>  {
>  #if ENABLE(JAVASCRIPT_DEBUGGER)
> -    m_page->corePage()->inspectorController()->showPanel(InspectorController::ScriptsPanel);
> -    m_page->corePage()->inspectorController()->enableDebugger();
> +    m_page->corePage()->inspectorController()->showAndEnableDebugger();
>  #endif
>  }

please use InspectorInstrumentation here.

[Pavel Feldman]

(In reply to comment #2)
> (From update of attachment 78989 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78989&action=review
> 
> > WebKit2/WebProcess/WebPage/WebInspector.cpp:104
> >  void WebInspector::startJavaScriptDebugging()
> >  {
> >  #if ENABLE(JAVASCRIPT_DEBUGGER)
> > -    m_page->corePage()->inspectorController()->showPanel(InspectorController::ScriptsPanel);
> > -    m_page->corePage()->inspectorController()->enableDebugger();
> > +    m_page->corePage()->inspectorController()->showAndEnableDebugger();
> >  #endif
> >  }
> 
> please use InspectorInstrumentation here.

This is not a part of the instrumentation, this is rather a control call form the WebKit to the inspector controller. A rare case where we need to talk to the 'inspector controller for host' interface (as opposed to 'inspector instrumentation for webcore' interface). At this moment, all such methods are defined on the inspector controller itself.

[Ilya Tikhonovsky]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 78989 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=78989&action=review
> > 
> > > WebKit2/WebProcess/WebPage/WebInspector.cpp:104
> > >  void WebInspector::startJavaScriptDebugging()
> > >  {
> > >  #if ENABLE(JAVASCRIPT_DEBUGGER)
> > > -    m_page->corePage()->inspectorController()->showPanel(InspectorController::ScriptsPanel);
> > > -    m_page->corePage()->inspectorController()->enableDebugger();
> > > +    m_page->corePage()->inspectorController()->showAndEnableDebugger();
> > >  #endif
> > >  }
> > 
> > please use InspectorInstrumentation here.
> 
> This is not a part of the instrumentation, this is rather a control call form the WebKit to the inspector controller. A rare case where we need to talk to the 'inspector controller for host' interface (as opposed to 'inspector instrumentation for webcore' interface). At this moment, all such methods are defined on the inspector controller itself.

this is preventing us remove InspectorController from the inspector's WebCore interface.
Right now we are using inspectorController as a master-agent in Frontend to Backend API and also as a part of Inspector WebKit/WebCore API. I think it'd be better to split these roles.

[Pavel Feldman]

> this is preventing us remove InspectorController from the inspector's WebCore interface.
> Right now we are using inspectorController as a master-agent in Frontend to Backend API and also as a part of Inspector WebKit/WebCore API. I think it'd be better to split these roles.

That's right. And that's why about a year ago I introduced 'InspectorBackend' interface that was covering  'inspector controller for the front-end' part of the API. Ironically, it was you to move all the stuff back into the inspector controller from the backend and it ended up nuked. I was reluctant to it back then and I think we should just re-introduce it now. InspectorBackend and InspectorController will stay highly coupled due to ability to turn on and of capabilities such as debugging, but that is Ok.

[Pavel Feldman]

To summarize:

- InspectorInstrumentation:: should only be used for instrumentation, not for controlling inspector, so comment #2 is wrong.
- The InspectorController vs InspectorBackend question is orthogonal to this change, it should be fixed separately.

[Ilya Tikhonovsky]

(In reply to comment #5)
> > this is preventing us remove InspectorController from the inspector's WebCore interface.
> > Right now we are using inspectorController as a master-agent in Frontend to Backend API and also as a part of Inspector WebKit/WebCore API. I think it'd be better to split these roles.
> 
> That's right. And that's why about a year ago I introduced 'InspectorBackend' interface that was covering  'inspector controller for the front-end' part of the API. Ironically, it was you to move all the stuff back into the inspector controller from the backend and it ended up nuked. I was reluctant to it back then and I think we should just re-introduce it now. InspectorBackend and InspectorController will stay highly coupled due to ability to turn on and of capabilities such as debugging, but that is Ok.

The plan described in 52510 is much better than previous try with InspectorBackend and I absolutely agree with this plan.

[Pavel Feldman]

> The plan described in 52510 is much better than previous try with InspectorBackend and I absolutely agree with this plan.

I agree. Now that we have instrumentation API and agents, it all looks much more solid.

[Yury Semikhatsky]

Comment on attachment 78989 [details]
[PATCH] Proposed change

View in context: https://bugs.webkit.org/attachment.cgi?id=78989&action=review

> Source/WebCore/inspector/InspectorController.cpp:1051
> +    if (debuggerEnabled())

Make sure that Scripts panel will be brought to front in case debugger was already enabled

[WebKit Commit Bot]

Comment on attachment 78989 [details]
[PATCH] Proposed change

Clearing flags on attachment: 78989

Committed r75939: <http://trac.webkit.org/changeset/75939>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.