Web Inspector: use InstrumentingAgents to access agents from InspectorInstrumentation.
Created attachment 93115 [details] Patch
Comment on attachment 93115 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93115&action=review > Source/WebCore/inspector/InspectorInstrumentation.cpp:457 > + if (!instrumentingAgents) Can this be 0? > Source/WebCore/inspector/InspectorInstrumentation.h:908 > + if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsWithFrontendForPage(page)) This does not make sense - instrumentingAgentsWithFrontend && hasFrontend? > Source/WebCore/inspector/InspectorInstrumentation.h:938 > + return instrumentingAgentsForFrame(frame); Now you need to test whether there is a front-end for the page. > Source/WebCore/inspector/InspectorInstrumentation.h:952 > + return instrumentingAgentsForContext(context); ditto > Source/WebCore/inspector/InspectorInstrumentation.h:958 > + return instrumentingAgentsForPage(document->page()); ditto
Comment on attachment 93115 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93115&action=review >> Source/WebCore/inspector/InspectorInstrumentation.cpp:457 >> + if (!instrumentingAgents) > > Can this be 0? Good question, I need to check. >> Source/WebCore/inspector/InspectorInstrumentation.h:908 >> + if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsWithFrontendForPage(page)) > > This does not make sense - instrumentingAgentsWithFrontend && hasFrontend? Why not? The first one only checks whether there is a front-end, not necessarily front-end for given page. >> Source/WebCore/inspector/InspectorInstrumentation.h:938 >> + return instrumentingAgentsForFrame(frame); > > Now you need to test whether there is a front-end for the page. I intentionally don't do this. hasFrontends() is fast check that makes the instrumentation seemless when there is no front-end. In all other cases the fast check will fail and we will do regular agent retrieval from InstrumentingAgents object. >> Source/WebCore/inspector/InspectorInstrumentation.h:952 >> + return instrumentingAgentsForContext(context); > > ditto See my reply above.
(In reply to comment #2) > (From update of attachment 93115 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93115&action=review > > Source/WebCore/inspector/InspectorInstrumentation.h:908 > > + if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsWithFrontendForPage(page)) > > This does not make sense - instrumentingAgentsWithFrontend && hasFrontend? The method should be renamed I think. It's called to check whether the HTML parser should report parse errors or not. It should be somethig like InspectorInstrumentation::shouldReportHtmlParseErrors. The fact that we need to call hasFrontends() to avoid possible overhead when there is no inspector window breaks our abstraction. Ideally, inspector instrumentation should not know anything about the front-end and should be able to get everything from InstrumentingAgents. Ilya suggests that we should remove the check completely and see whether it would result in a observable performance degradation.
Created attachment 93636 [details] Patch
(In reply to comment #5) > Created an attachment (id=93636) [details] > Patch Removed all *WithFrontend* methods and added a macros for fast return when there are no inspector frontends.
Comment on attachment 93636 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93636&action=review > Source/WebCore/inspector/InspectorInstrumentation.h:715 > + if (hasFrontends()) replace with the macro? > Source/WebCore/inspector/InspectorInstrumentation.h:723 > + if (hasFrontends()) ditto > Source/WebCore/inspector/InspectorInstrumentation.h:731 > + if (hasFrontends()) ditto
Created attachment 93637 [details] Patch for landing Comments addressed.
Committed r86564: <http://trac.webkit.org/changeset/86564>