Bug 73225 – Web Inspector: put inspector agents into a vector in the InspectorController.

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

[Pavel Feldman]

Patch to follow.

[Pavel Feldman]

Created attachment 116773 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 116773 [details]
Patch

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

> Source/WebCore/ChangeLog:5
> +

Please describe the change here.

> Source/WebCore/ChangeLog:99
> +        Web Inspector: align agent creation (factory vs constructors).

Remove this entry.

> Source/WebCore/inspector/InspectorBaseAgent.h:42
> +class InspectorBaseAgentInterface {

Consider moving it into its own file.

> Source/WebCore/inspector/InspectorBaseAgent.h:67
> +        , m_agent(static_cast<T*>(this))

You don't need to keep it in a field, just apply this cast in registerDispatcher.

> Source/WebCore/inspector/InspectorController.h:34
> +#include "InspectorBaseAgent.h"

Forward declaration should suffice

[Pavel Feldman]

> > Source/WebCore/inspector/InspectorBaseAgent.h:42
> > +class InspectorBaseAgentInterface {
> 
> Consider moving it into its own file.
> 

I consider InspectorBaseAgentInterface a part of the InspectroBaseAgent. It is just a convenience way for storing all agents in the same vector while implementing double dispatch using templates.

[Ilya Tikhonovsky]

Comment on attachment 116773 [details]
Patch

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

> Source/WebCore/inspector/CodeGeneratorInspector.py:1267
> +            Generator.backend_setters_list.append("    void registerDispatcher(%s* %s) { m_%s = %s; }" % (agent_type_name, agent_field_name, agent_field_name, agent_field_name))

backend_agent_setters_list?
registerAgent?
I'd add ASSERT(!m_%s) here and m_%s = 0 in constructor.

> Source/WebCore/inspector/InspectorBaseAgent.h:34
> +#include "InspectorBackendDispatcher.h"

It is bad dependency.

> Source/WebCore/inspector/InspectorBaseAgent.h:60
> +        dispatcher->registerDispatcher(m_agent);

Looks like you can do static cast right here and remove m_agent member.

[Pavel Feldman]

Created attachment 116916 [details]
[Patch] review comments addressed.

[Yury Semikhatsky]

Comment on attachment 116916 [details]
[Patch] review comments addressed.

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

> Source/WebCore/inspector/InspectorController.cpp:162
> +        (*it)->inspectedPageDestroyed();

You don't need this method.

[Pavel Feldman]

Created attachment 116921 [details]
[Patch] to land

[Pavel Feldman]

Committed r101345: <http://trac.webkit.org/changeset/101345>

[Adam Klein]

(In reply to comment #8)
> Committed r101345: <http://trac.webkit.org/changeset/101345>

This change appears to be triggering errors on the Valgrind bots:

http://build.chromium.org/p/chromium.webkit/builders/Linux%20Valgrind/builds/17144/steps/memory%20test%3A%20test_shell/logs/stdio

I've filed a bug with details and a suggested fix: https://bugs.webkit.org/show_bug.cgi?id=73582