[webkit-reviews] review granted: [Bug 226624] Web Inspector: Add instrumentation to node destruction for InspectorDOMAgent : [Attachment 431219] Patch v2.3 - Add valid key check to InspectorDOMAgent::boundNodeId

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Patrick Angle <pangle at apple.com>'s
request for review:
Bug 226624: Web Inspector: Add instrumentation to node destruction for
InspectorDOMAgent
https://bugs.webkit.org/show_bug.cgi?id=226624

Attachment 431219: Patch v2.3 - Add valid key check to
InspectorDOMAgent::boundNodeId

https://bugs.webkit.org/attachment.cgi?id=431219&action=review




--- Comment #14 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 431219
  --> https://bugs.webkit.org/attachment.cgi?id=431219
Patch v2.3 - Add valid key check to InspectorDOMAgent::boundNodeId

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

r=mews, awesome awesome awesome

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:382
> +Protocol::DOM::NodeId InspectorDOMAgent::bind(Node* node)

NIT: I wonder if we should make this into `Node&` while here so that we know
for sure that `m_nodeToId.ensure` won't complain about an invalid key (e.g. not
`nullptr`).  Alternatively you could check it just like `boundNodeId`.

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:391
> +void InspectorDOMAgent::unbind(Node* node)

ditto (:382)

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2367
> +    auto prevId = previousSibling ? boundNodeId(previousSibling) : 0;

you can drop the `previousSibling ? ` check now that `boundNodeId` does it for
you

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2425
>      if (!parent)

ditto (:2367)

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2439
> +	   auto prevId = prevSibling ? boundNodeId(prevSibling) : 0;

ditto (:2367)

> Source/WebCore/inspector/agents/page/PageConsoleAgent.cpp:56
>  Protocol::ErrorStringOr<void> PageConsoleAgent::clearMessages()

you can just remove this function entirely now