Bug 56389 – Web Inspector: split InspectorAgent.populateScriptObjects into more granular agent-specific requests

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

[Yury Semikhatsky]

Web Inspector: split InspectorAgent.populateScriptObjects into more granular agent-specific requests

[Yury Semikhatsky]

Created attachment 85828 [details]
Patch

[Pavel Feldman]

Comment on attachment 85828 [details]
Patch

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

All looks good except for the getPreferredPanel. We should guess by the event / action.

> Source/WebCore/inspector/Inspector.idl:37
> +        void getPreferredPanel(out String panel);

I don't particularly like it. Inspector should determine preferred panel by itself based on the event that is happening.

> Source/WebCore/inspector/InspectorAgent.cpp:459
>  void InspectorAgent::pushDataCollectedOffline()

It should be renamed to pushWorkers for now? Can we also push them from within debugger::enable instead?

> Source/WebCore/inspector/InspectorDOMAgent.cpp:292
> +    m_document = 0;

Comment would be nice.

> Source/WebCore/inspector/front-end/ProfilesPanel.js:141
> +        ProfilerAgent.isEnabled(function(error, value) {

Please declare a named function.

[Yury Semikhatsky]

Created attachment 85909 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #2)
> (From update of attachment 85828 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85828&action=review
> 
> All looks good except for the getPreferredPanel. We should guess by the event / action.
> 
Good point. Replaced it with the showPanel event which is already used when the frontend is open.


> > Source/WebCore/inspector/Inspector.idl:37
> > +        void getPreferredPanel(out String panel);
> 
> I don't particularly like it. Inspector should determine preferred panel by itself based on the event that is happening.
> 
See above.

> > Source/WebCore/inspector/InspectorAgent.cpp:459
> >  void InspectorAgent::pushDataCollectedOffline()
> 
> It should be renamed to pushWorkers for now? Can we also push them from within debugger::enable instead?
> 
The code has been inlined into InspectorAgent::setFrontend since it's the only place where it was called. I don't like to move this into the debugger agent now for several reasons: 1) debugger instrumentation is off when there is no front-end while workers data is collected in that case too 2) this code would introduce additional dependency on the inspected page in the debugger agent 3) it will go away once we have native debugger for workers.

> > Source/WebCore/inspector/InspectorDOMAgent.cpp:292
> > +    m_document = 0;
> 
> Comment would be nice.
> 
Done.

> > Source/WebCore/inspector/front-end/ProfilesPanel.js:141
> > +        ProfilerAgent.isEnabled(function(error, value) {
> 
> Please declare a named function.
Done.

[Pavel Feldman]

Comment on attachment 85909 [details]
Patch

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

> Source/WebCore/inspector/InspectorAgent.cpp:401
> +        m_frontend->inspector()->showPanel(m_showPanelAfterVisible);

Just an FYI, no action required: I am still not a fan of it. We should make WebInspector contribute to the Develop menu instead. Hence we would not need to be involved with the panel names on the backend side. Maybe a FIXME here.

[Yury Semikhatsky]

Committed r81222: <http://trac.webkit.org/changeset/81222>

[Yury Semikhatsky]

Comment on attachment 85909 [details]
Patch

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

>> Source/WebCore/inspector/InspectorAgent.cpp:401
>> +        m_frontend->inspector()->showPanel(m_showPanelAfterVisible);
> 
> Just an FYI, no action required: I am still not a fan of it. We should make WebInspector contribute to the Develop menu instead. Hence we would not need to be involved with the panel names on the backend side. Maybe a FIXME here.

Filed a bug on this: https://bugs.webkit.org/show_bug.cgi?id=56451