Patch to follow.
Created attachment 78989 [details] [PATCH] Proposed change
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.
(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.
(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.
> 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.
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.
(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.
> 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.
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
Comment on attachment 78989 [details] [PATCH] Proposed change Clearing flags on attachment: 78989 Committed r75939: <http://trac.webkit.org/changeset/75939>
All reviewed patches have been landed. Closing bug.