-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Adopt smart pointers in RemoteWebInspectorUI.cpp #47322
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Adopt smart pointers in RemoteWebInspectorUI.cpp #47322
Conversation
EWS run on previous version of this PR (hash 0e47b0a) |
Source/WebCore/page/Page.cpp
Outdated
@@ -1591,6 +1591,11 @@ DiagnosticLoggingClient& Page::diagnosticLoggingClient() const | |||
return *m_diagnosticLoggingClient; | |||
} | |||
|
|||
CheckedRef<DiagnosticLoggingClient> Page::protectedDiagnosticLoggingClient() const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can call this member function 'checkedDiagnosticLoggingClient()'
@@ -72,7 +72,7 @@ void RemoteWebInspectorUI::initialize(DebuggableInfoData&& debuggableInfo, const | |||
m_debuggableInfo = WTFMove(debuggableInfo); | |||
m_backendCommandsURL = backendCommandsURL; | |||
|
|||
m_page->protectedCorePage()->inspectorController().setInspectorFrontendClient(this); | |||
WeakRef { m_page }->protectedCorePage()->inspectorController().setInspectorFrontendClient(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we can create a helper method: Ref<WebPage> RemoteWebInspectorUI::protectedWebPage()
?
0e47b0a
to
6bc4dc3
Compare
EWS run on previous version of this PR (hash 6bc4dc3) |
|
||
m_frontendHost = InspectorFrontendHost::create(this, m_page->protectedCorePage().get()); | ||
m_frontendHost->addSelfToGlobalObjectInWorld(mainThreadNormalWorldSingleton()); | ||
frontendHost = InspectorFrontendHost::create(this, protectedWebPage()->protectedCorePage().get()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should still assign this to m_frontendHost
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh that's so careless of me! Fixing.
Safer C++ Build #41906 (0e47b0a)❌ Found 1 failing file with 4 issues. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming. |
6bc4dc3
to
b4952c5
Compare
EWS run on previous version of this PR (hash b4952c5) |
b4952c5
to
5c7ff4c
Compare
EWS run on previous version of this PR (hash 5c7ff4c) |
5c7ff4c
to
5ba16b5
Compare
EWS run on previous version of this PR (hash 5ba16b5) |
@@ -90,16 +90,16 @@ void RemoteWebInspectorUI::sendMessageToFrontend(const String& message) | |||
|
|||
void RemoteWebInspectorUI::sendMessageToBackend(const String& message) | |||
{ | |||
WebProcess::singleton().parentProcessConnection()->send(Messages::RemoteWebInspectorUIProxy::SendMessageToBackend(message), m_page->identifier()); | |||
WebProcess::singleton().protectedParentProcessConnection()->send(Messages::RemoteWebInspectorUIProxy::SendMessageToBackend(message), m_page->identifier()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit silly. parentProcessConnection is a singleton just like WebProcess::singleton()
. Maybe we can wrap in a helper function by the name of parentProcessConnectionSingleton()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. There's a lot of usages of protectedParentProcessConnection() outside of this file. Should I file a bugzilla ticket to replace all of those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talked offline. We will be leaving this as-is.
Safer C++ Build #41918 (b4952c5)❌ Found 1 failing file with 4 issues. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming. |
Commit message contains (OOPS!), blocking PR #47322. Details: Build #17705 |
5ba16b5
to
47adbd8
Compare
EWS run on previous version of this PR (hash 47adbd8) |
@@ -35,8 +35,9 @@ | |||
|
|||
namespace WebKit { | |||
|
|||
class DiagnosticLoggingClient final : public API::DiagnosticLoggingClient { | |||
class DiagnosticLoggingClient final : public API::DiagnosticLoggingClient, public CanMakeCheckedPtr<DiagnosticLoggingClient> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this change? Where is this used in this PR? I looked at the PR twice but couldn't see any code using a CheckedPtr to a WebKit::DiagnosticLoggingClient
, only to the WebCore one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I needed DiagnosticLoggingClient to become a CheckedPtr in order to make this helper function:
Source/WebCore/page/Page.h:
WEBCORE_EXPORT CheckedRef<DiagnosticLoggingClient> checkedDiagnosticLoggingClient() const;`
which I eventually used in
Source/WebKit/WebProcess/Inspector/RemoteWebInspectorUI.cpp:
void RemoteWebInspectorUI::logDiagnosticEvent(const String& eventName, const DiagnosticLoggingClient::ValueDictionary& dictionary)
{
..
protectedWebPage()->protectedCorePage()->checkedDiagnosticLoggingClient()->logDiagnosticMessageWithValueDictionary(eventName, "Remote Web Inspector Frontend Diagnostics"_s, dictionary, ShouldSample::No);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chris is pointing out that we have both Source/WebCore/page/DiagnosticLoggingClient.h
and Source/WebKit/UIProcess/Cocoa/DiagnosticLoggingClient.h
. And that you're not using the latter in this patch. And the former already has CanMakeCheckedPtr
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhh, I didn't even realize that there were two separate DiagnosticLoggingClient headers... Now that I'm reading Chris's comment closer - he's talking about a Webcore vs a Webkit one.
Anyways, even I was using the wrong file, I should have double-checked if the parent class was ref'd or check'd.
You two are right. I don't need this change at all since I'm using the Webcore one not the Webkit. I'll back this out.
|
||
WeakRef<WebPage> m_page; | ||
const WeakRef<WebPage> m_page; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. Don't need it as we discussed. I forgot about this and will back it out.
You need to drop this OOPS line. |
https://bugs.webkit.org/show_bug.cgi?id=295068 rdar://problem/154521813 Reviewed by NOBODY (OOPS!). Smart pointer adoption as per the static analyzer. * Source/WebCore/page/Page.cpp: (WebCore::Page::checkedDiagnosticLoggingClient const): * Source/WebCore/page/Page.h: * Source/WebKit/SaferCPPExpectations/UncheckedCallArgsCheckerExpectations: * Source/WebKit/SaferCPPExpectations/UncountedCallArgsCheckerExpectations: * Source/WebKit/WebProcess/Inspector/RemoteWebInspectorUI.cpp: (WebKit::RemoteWebInspectorUI::RemoteWebInspectorUI): (WebKit::RemoteWebInspectorUI::initialize): (WebKit::RemoteWebInspectorUI::sendMessageToBackend): (WebKit::RemoteWebInspectorUI::windowObjectCleared): (WebKit::RemoteWebInspectorUI::frontendLoaded): (WebKit::RemoteWebInspectorUI::changeSheetRect): (WebKit::RemoteWebInspectorUI::setForcedAppearance): (WebKit::RemoteWebInspectorUI::startWindowDrag): (WebKit::RemoteWebInspectorUI::bringToFront): (WebKit::RemoteWebInspectorUI::closeWindow): (WebKit::RemoteWebInspectorUI::reopen): (WebKit::RemoteWebInspectorUI::resetState): (WebKit::RemoteWebInspectorUI::openURLExternally): (WebKit::RemoteWebInspectorUI::revealFileExternally): (WebKit::RemoteWebInspectorUI::save): (WebKit::RemoteWebInspectorUI::load): (WebKit::RemoteWebInspectorUI::pickColorFromScreen): (WebKit::RemoteWebInspectorUI::showCertificate): (WebKit::RemoteWebInspectorUI::setInspectorPageDeveloperExtrasEnabled): (WebKit::RemoteWebInspectorUI::logDiagnosticEvent): (WebKit::RemoteWebInspectorUI::protectedWebPage): * Source/WebKit/WebProcess/Inspector/RemoteWebInspectorUI.h:
47adbd8
to
723cfc8
Compare
EWS run on current version of this PR (hash 723cfc8) |
723cfc8
723cfc8
🛠 playstation