Bug 47173 – Web Inspector: extract Inspector Instrumentation API as a class

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

[Pavel Podivilov]

Web Inspector: extract Inspector Instrumentation API as a class

Remove references to InspectorTimelineAgent from WebCore.

[Pavel Podivilov]

Created attachment 69777 [details]
Patch.

[Pavel Podivilov]

Created attachment 69783 [details]
Patch.

Comments addressed.

[Pavel Podivilov]

(In reply to comment #2)
> Created an attachment (id=69783) [details]
> Patch.
> 
> Comments addressed.

Wrong patch, sorry.

[Ilya Tikhonovsky]

Comment on attachment 69777 [details]
Patch.

looks good to me

[Ilya Tikhonovsky]

Comment on attachment 69777 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=69777&action=review

> WebCore/dom/Node.cpp:2605
> +    int instrumentationCookie = InspectorInstrumentation::willDispatchEvent(document(), *event, targetForWindowEvents, this, ancestors);

Nit: InspectorInstrumentationCookie cookie?

> WebCore/inspector/InspectorInstrumentation.cpp:313
> +    if (InspectorTimelineAgent* timelineAgent = inspectorController->m_timelineAgent.get()) {

Nit: I think it'd be better to have overloaded function for retrieving timelineAgent. like retrieveTimelineAgent(InspectorController*)

[Pavel Podivilov]

Created attachment 69953 [details]
Comments addressed.

[Pavel Feldman]

Comment on attachment 69953 [details]
Comments addressed.

View in context: https://bugs.webkit.org/attachment.cgi?id=69953&action=review

> WebCore/ChangeLog:27
> +        (WebCore::InspectorInstrumentation::willInsertDOMNodeImpl):

You should nuke these scary verbose lines.

> WebCore/bindings/v8/V8Proxy.cpp:414
> +    InspectorInstrumentation::didEvaluateScript(m_frame, cookie);

I think cookie should be the first parameter in all the calls.

> WebCore/inspector/InspectorInstrumentation.cpp:-67
> -    if (!inspectorController->hasFrontend())

Why did these go? Please explain in order to get r+.

> WebCore/inspector/InspectorInstrumentation.h:116
> +    static InspectorInstrumentationCookie willCallFunctionImpl(InspectorController*, const String& scriptName, int scriptLine);

This is crazy amount of methods, really easy to make a mistake here. Can we cut the number?

[Pavel Podivilov]

> 
> > WebCore/inspector/InspectorInstrumentation.cpp:-67
> > -    if (!inspectorController->hasFrontend())
> 
> Why did these go? Please explain in order to get r+.

This checks were moved to InspectorInstrumentation::inspectorControllerForPage method.

[Pavel Feldman]

Comment on attachment 69953 [details]
Comments addressed.

View in context: https://bugs.webkit.org/attachment.cgi?id=69953&action=review

> WebCore/inspector/InspectorInstrumentation.h:61
> +    static void willSendXMLHttpRequest(ScriptExecutionContext*, const String& url);

LGTM given we migrate to a macro shortly.

[Pavel Podivilov]

Committed r69283: <http://trac.webkit.org/changeset/69283>

[Kent Tamura]

This change broke https://bugs.webkit.org/show_bug.cgi?id=47173 on all platforms.

[Kent Tamura]

(In reply to comment #11)
> This change broke https://bugs.webkit.org/show_bug.cgi?id=47173 on all platforms.

Oops, copy&paste error.

This change broke  http/tests/inspector/console-xhr-logging.html on all platforms.

[Pavel Podivilov]

(In reply to comment #12)
> (In reply to comment #11)
> > This change broke https://bugs.webkit.org/show_bug.cgi?id=47173 on all platforms.
> 
> Oops, copy&paste error.
> 
> This change broke  http/tests/inspector/console-xhr-logging.html on all platforms.

Fixed in r69286.