Bug 60624 – Web Inspector: use InstrumentingAgents to access agents from InspectorInstrumentation

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

[Yury Semikhatsky]

Web Inspector: use InstrumentingAgents to access agents from InspectorInstrumentation.

[Yury Semikhatsky]

Created attachment 93115 [details]
Patch

[Pavel Feldman]

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

[Yury Semikhatsky]

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.

[Yury Semikhatsky]

(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.

[Yury Semikhatsky]

Created attachment 93636 [details]
Patch

[Yury Semikhatsky]

(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.

[Pavel Feldman]

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

[Yury Semikhatsky]

Created attachment 93637 [details]
Patch for landing

Comments addressed.

[Yury Semikhatsky]

Committed r86564: <http://trac.webkit.org/changeset/86564>