Bug 56912 – Web Inspector: move node searching and node highlight related methods from InspectorAgent to DOMAgent

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

[Ilya Tikhonovsky]

%subj%

[Ilya Tikhonovsky]

Created attachment 86594 [details]
Patch

[WebKit Review Bot]

Attachment 86594 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8230495

[Ilya Tikhonovsky]

Created attachment 86597 [details]
Patch

[Build Bot]

Attachment 86594 [details] did not build on win:
Build output: http://queues.webkit.org/results/8227735

[Yury Semikhatsky]

Comment on attachment 86597 [details]
Patch

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

> Source/WebCore/inspector/DOMNodeHighlighter.cpp:216
> +} // unnamed namespace

unnamed -> anonymous or just // namespace ?

> Source/WebCore/inspector/InspectorAgent.cpp:127
> +    m_instrumentingAgents->setInspectorAgent(this);

There should be symmetric call reseting InspectorAgent.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:100
> +static const char searchingForNode[] = "searchingForNode";

Is it still in use?

> Source/WebCore/inspector/InspectorDOMAgent.cpp:917
> +bool InspectorDOMAgent::searchingForNodeInPage() const

No need to expose this through a public method, just use m_searchingForNode directly in this class.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:161
> +    if (!inspectorAgent->enabled())

You wouldn't need this check if you used instrumentingAgents()->inspectorDOMAgent(), please fix.

[Ilya Tikhonovsky]

Committed r81771: <http://trac.webkit.org/changeset/81771>

[Pavel Feldman]

Comment on attachment 86597 [details]
Patch

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

This is r- for several reasons, I wonder how it ended up with r+.

> Source/WebCore/inspector/DOMNodeHighlighter.h:37
> +namespace DOMNodeHighlighter {

Why custom namespace? What is wrong with using WebCore? This is not standard approach afaik.

> Source/WebCore/inspector/DOMNodeHighlighter.h:39
> +void DrawNodeHighlight(GraphicsContext&, Node*);

Methods in WebCore start with lower case.

> Source/WebCore/inspector/Inspector.json:676
> +                "name": "setSearchingForNode",

DOM domain is documented, you should not introduce commands that are not documented into it.

[Pavel Feldman]

Comment on attachment 86597 [details]
Patch

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

> Source/WebCore/inspector/Inspector.json:696
> +                    { "name": "frameId", "type": "string" }

Hm. DOM domain does not operate frameIds. This is the term from a different "Network" domain. This command does not belong here.

[Pavel Feldman]

Comment on attachment 86597 [details]
Patch

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

> Source/WebCore/inspector/InspectorDOMAgent.cpp:65
> +#include "InspectorAgent.h"

1) I think it is very wrong to add a dependency from InspectorDOMAgent to InspectorResourceAgent.
2) I also think it is very wrong to put dependency from any agent to InspectorAgent

I think reasons above are sufficient to roll this change out.

[Pavel Feldman]

(In reply to comment #9)
> (From update of attachment 86597 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86597&action=review
> 
> > Source/WebCore/inspector/InspectorDOMAgent.cpp:65
> > +#include "InspectorAgent.h"
> 
> 1) I think it is very wrong to add a dependency from InspectorDOMAgent to InspectorResourceAgent.

read as of InspectorDOMAgent from InspectorResourceAgent

> 2) I also think it is very wrong to put dependency from any agent to InspectorAgent
> 

read as of any agent from InspectorAgent

> I think reasons above are sufficient to roll this change out.

Morning brains confused, blood boils :)