.
<rdar://problem/85526508>
Created attachment 444606 [details] Patch v1.0
Comment on attachment 444606 [details] Patch v1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=444606&action=review > Source/WebKit/ChangeLog:12 > + Other operations using m_inspectedPage should be guarded in case it tha the inspected oops > Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.cpp:421 > + m_inspectedPageIdentifier = m_inspectedPage->identifier(); Why are we only saving this here? Why not right after when `m_inspectedPage` is set (e.g. `WebInspectorUIProxy::updateForNewPageProcess`)? Also, when do we clear this? Maybe when we clear `m_inspectedPage` (e.g. `WebInspectorUIProxy::reset`)? > Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.cpp:423 > + m_inspectorPage->process().addMessageReceiver(Messages::WebInspectorUIProxy::messageReceiverName(), m_inspectedPageIdentifier, *this); Should we also do the same thing for the use in `WebInspectorUIProxy::openLocalInspectorFrontend`? > Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.h:292 > + WebPageProxyIdentifier m_inspectedPageIdentifier { }; I don't think the `{ }` is needed.
(In reply to Devin Rousso from comment #3) > Comment on attachment 444606 [details] > Patch v1.0 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=444606&action=review > > > Source/WebKit/ChangeLog:12 > > + Other operations using m_inspectedPage should be guarded in case it tha the inspected > > oops > > > Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.cpp:421 > > + m_inspectedPageIdentifier = m_inspectedPage->identifier(); > > Why are we only saving this here? Why not right after when > `m_inspectedPage` is set (e.g. > `WebInspectorUIProxy::updateForNewPageProcess`)? > > Also, when do we clear this? Maybe when we clear `m_inspectedPage` (e.g. > `WebInspectorUIProxy::reset`)? Good suggestion, I fixed it. > > Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.cpp:423 > > + m_inspectorPage->process().addMessageReceiver(Messages::WebInspectorUIProxy::messageReceiverName(), m_inspectedPageIdentifier, *this); > > Should we also do the same thing for the use in > `WebInspectorUIProxy::openLocalInspectorFrontend`? I tried changing it to use the cached identifier, but it resulted in "Message not understood: EstablishConnection". We shouldn't change the cached identifier after setting it because the same destination is needed to add and remove the message receiver. So, I think it is fine to re-query here as the code currently does.
Comment on attachment 444606 [details] Patch v1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=444606&action=review >>> Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.cpp:423 >>> + m_inspectorPage->process().addMessageReceiver(Messages::WebInspectorUIProxy::messageReceiverName(), m_inspectedPageIdentifier, *this); >> >> Should we also do the same thing for the use in `WebInspectorUIProxy::openLocalInspectorFrontend`? > > I tried changing it to use the cached identifier, but it resulted in "Message not understood: EstablishConnection". We shouldn't change the cached identifier after setting it because the same destination is needed to add and remove the message receiver. > > So, I think it is fine to re-query here as the code currently does. Err, I meant more to use `m_inspectedPageIdentifier` instead of `m_inspectedPage->identifier()`. I would hope that the identifier doesn't change in between the call to `createFrontendPage` and sending `WebInspectorUI::EstablishConnection` 😅
Created attachment 445788 [details] Patch v1.1 (for landing)
Comment on attachment 445788 [details] Patch v1.1 (for landing) View in context: https://bugs.webkit.org/attachment.cgi?id=445788&action=review r=me, nice! =D > Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.cpp:424 > + m_inspectorPage->process().addMessageReceiver(Messages::WebInspectorUIProxy::messageReceiverName(), m_inspectedPageIdentifier, *this); please see comment #5 :)
Committed r286498 (244837@main): <https://commits.webkit.org/244837@main>