Bug 60624 - Web Inspector: use InstrumentingAgents to access agents from InspectorInstrumentation
Summary: Web Inspector: use InstrumentingAgents to access agents from InspectorInstrum...
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: Yury Semikhatsky
URL:
Keywords:
Depends on: 60359
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-11 07:09 PDT by Yury Semikhatsky
Modified: 2011-05-16 06:28 PDT (History)
10 users (show)

See Also:


Attachments
Patch (125.66 KB, patch)
2011-05-11 07:27 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (127.14 KB, patch)
2011-05-16 05:34 PDT, Yury Semikhatsky
pfeldman: review+
pfeldman: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (131.14 KB, patch)
2011-05-16 06:25 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Semikhatsky 2011-05-11 07:09:22 PDT
Web Inspector: use InstrumentingAgents to access agents from InspectorInstrumentation.
Comment 1 Yury Semikhatsky 2011-05-11 07:27:30 PDT
Created attachment 93115 [details]
Patch
Comment 2 Pavel Feldman 2011-05-11 08:25:53 PDT
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 3 Yury Semikhatsky 2011-05-11 11:52:32 PDT
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.
Comment 4 Yury Semikhatsky 2011-05-13 08:35:24 PDT
(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.
Comment 5 Yury Semikhatsky 2011-05-16 05:34:34 PDT
Created attachment 93636 [details]
Patch
Comment 6 Yury Semikhatsky 2011-05-16 05:52:17 PDT
(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 7 Pavel Feldman 2011-05-16 06:02:46 PDT
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
Comment 8 Yury Semikhatsky 2011-05-16 06:25:51 PDT
Created attachment 93637 [details]
Patch for landing

Comments addressed.
Comment 9 Yury Semikhatsky 2011-05-16 06:28:19 PDT
Committed r86564: <http://trac.webkit.org/changeset/86564>