Web Inspector: split InspectorAgent.populateScriptObjects into more granular agent-specific requests
Created attachment 85828 [details] Patch
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.
Created attachment 85909 [details] Patch
(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.
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.
Committed r81222: <http://trac.webkit.org/changeset/81222>
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