[webkit-reviews] review granted: [Bug 56912] Web Inspector: move node searching and node highlight related methods from InspectorAgent to DOMAgent : [Attachment 86597] Patch

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys at chromium.org> has granted Ilya Tikhonovsky
<loislo at chromium.org>'s request for review:
Bug 56912: Web Inspector: move node searching and node highlight related
methods from InspectorAgent to DOMAgent
https://bugs.webkit.org/show_bug.cgi?id=56912

Attachment 86597: Patch
https://bugs.webkit.org/attachment.cgi?id=86597&action=review

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
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.