WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
60624
Web Inspector: use InstrumentingAgents to access agents from InspectorInstrumentation
https://bugs.webkit.org/show_bug.cgi?id=60624
Summary
Web Inspector: use InstrumentingAgents to access agents from InspectorInstrum...
Yury Semikhatsky
Reported
2011-05-11 07:09:22 PDT
Web Inspector: use InstrumentingAgents to access agents from InspectorInstrumentation.
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Yury Semikhatsky
Comment 1
2011-05-11 07:27:30 PDT
Created
attachment 93115
[details]
Patch
Pavel Feldman
Comment 2
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
Yury Semikhatsky
Comment 3
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.
Yury Semikhatsky
Comment 4
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.
Yury Semikhatsky
Comment 5
2011-05-16 05:34:34 PDT
Created
attachment 93636
[details]
Patch
Yury Semikhatsky
Comment 6
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.
Pavel Feldman
Comment 7
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
Yury Semikhatsky
Comment 8
2011-05-16 06:25:51 PDT
Created
attachment 93637
[details]
Patch for landing Comments addressed.
Yury Semikhatsky
Comment 9
2011-05-16 06:28:19 PDT
Committed
r86564
: <
http://trac.webkit.org/changeset/86564
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug