Bug 52472 - Web Inspector: simplify debugger enabling routine.
Summary: Web Inspector: simplify debugger enabling routine.
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: Pavel Feldman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-14 13:06 PST by Pavel Feldman
Modified: 2011-01-17 06:31 PST (History)
11 users (show)

See Also:


Attachments
[PATCH] Proposed change (11.04 KB, patch)
2011-01-14 13:38 PST, Pavel Feldman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2011-01-14 13:06:14 PST
Patch to follow.
Comment 1 Pavel Feldman 2011-01-14 13:38:48 PST
Created attachment 78989 [details]
[PATCH] Proposed change
Comment 2 Ilya Tikhonovsky 2011-01-14 14:04:56 PST
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.
Comment 3 Pavel Feldman 2011-01-14 22:57:25 PST
(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.
Comment 4 Ilya Tikhonovsky 2011-01-15 00:05:57 PST
(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.
Comment 5 Pavel Feldman 2011-01-15 03:14:44 PST
> 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.
Comment 6 Pavel Feldman 2011-01-15 03:19:59 PST
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.
Comment 7 Ilya Tikhonovsky 2011-01-15 06:02:01 PST
(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.
Comment 8 Pavel Feldman 2011-01-15 08:48:58 PST
> 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 9 Yury Semikhatsky 2011-01-17 04:58:37 PST
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 10 WebKit Commit Bot 2011-01-17 06:31:34 PST
Comment on attachment 78989 [details]
[PATCH] Proposed change

Clearing flags on attachment: 78989

Committed r75939: <http://trac.webkit.org/changeset/75939>
Comment 11 WebKit Commit Bot 2011-01-17 06:31:39 PST
All reviewed patches have been landed.  Closing bug.