Skip to content

Adopt smart pointers in WebFullScreenManagerProxy.cpp, WebInspectorClient.cpp, WebInspectorInternal.cpp #47411

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Adopt smart pointers in WebFullScreenManagerProxy.cpp, WebInspectorClient.cpp, WebInspectorInternal.cpp #47411

wants to merge 1 commit into from
+67 −38

Conversation

jelee53
Copy link
Contributor

@jelee53 jelee53 commented Jun 30, 2025 •
edited by webkit-early-warning-system
Loading

51d2c6a

Adopt smart pointers in WebFullScreenManagerProxy.cpp, WebInspectorClient.cpp, WebInspectorInternal.cpp
https://bugs.webkit.org/show_bug.cgi?id=295243
rdar://154710862

Reviewed by NOBODY (OOPS!).

Smart pointer adoption as per the static analyzer.

* Source/WebKit/SaferCPPExpectations/UncheckedCallArgsCheckerExpectations:
* Source/WebKit/SaferCPPExpectations/UncountedCallArgsCheckerExpectations:
* Source/WebKit/UIProcess/WebFullScreenManagerProxy.cpp:
(WebKit::WebFullScreenManagerProxy::isFullScreen):
* Source/WebKit/WebProcess/Inspector/WebInspectorClient.cpp:
(WebKit::WebInspectorClient::~WebInspectorClient):
(WebKit::WebInspectorClient::openLocalFrontend):
(WebKit::WebInspectorClient::bringFrontendToFront):
(WebKit::WebInspectorClient::didResizeMainFrame):
(WebKit::WebInspectorClient::showPaintRect):
(WebKit::WebInspectorClient::elementSelectionChanged):
(WebKit::WebInspectorClient::timelineRecordingChanged):
(WebKit::WebInspectorClient::setDeveloperPreferenceOverride):
* Source/WebKit/WebProcess/Inspector/WebInspectorInternal.cpp:
(WebKit::WebInspector::~WebInspector):
(WebKit::WebInspector::openLocalInspectorFrontend):
(WebKit::WebInspector::closeFrontendConnection):
(WebKit::WebInspector::bringToFront):
(WebKit::WebInspector::showMainResourceForFrame):
(WebKit::WebInspector::elementSelectionChanged):
(WebKit::WebInspector::timelineRecordingChanged):
(WebKit::WebInspector::setDeveloperPreferenceOverride):
(WebKit::WebInspector::setEmulatedConditions):
(WebKit::WebInspector::canAttachWindow):
(WebKit::WebInspector::updateDockingAvailability):
* Source/WebKit/WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::protectedInspector):
(WebKit::WebPage::protectedInspectorUI):
(WebKit::WebPage::protectedRemoteInspectorUI):
* Source/WebKit/WebProcess/WebPage/WebPage.h:

51d2c6a

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style loading 🛠 ios loading 🛠 mac loading 🛠 wpe ⏳ 🛠 win
loading 🛠 ios-sim ✅ 🛠 mac-AS-debug loading 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl loading 🧪 ios-wk2 loading 🧪 api-mac loading 🧪 api-wpe
loading 🧪 ios-wk2-wpt loading 🧪 mac-wk1 loading 🛠 wpe-cairo
loading 🧪 api-ios loading 🧪 mac-wk2 ⏳ 🛠 gtk
loading 🛠 vision ⏳ 🧪 mac-AS-debug-wk2 ⏳ 🧪 gtk-wk2
loading 🛠 vision-sim loading 🧪 mac-wk2-stress ⏳ 🧪 api-gtk
⏳ 🧪 vision-wk2 loading 🧪 mac-intel-wk2 ⏳ 🛠 playstation
loading 🛠 tv loading 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
⏳ 🛠 watch-sim

@jelee53 jelee53 self-assigned this Jun 30, 2025
@jelee53 jelee53 added the WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore). label Jun 30, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 30, 2025 •
edited
Loading

EWS run on previous version of this PR (hash ee580ee)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2   🛠 playstation
✅ 🛠 tv ❌ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@jelee53 jelee53 requested review from pvollan and annevk July 1, 2025 00:10
@webkit-ews-buildbot
Copy link
Collaborator

Safer C++ Build #42166 (ee580ee)

❌ Found 2 failing files with 15 issues. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming.
(cc @rniwa)

@jelee53
Copy link
Contributor Author

jelee53 commented Jul 1, 2025

Didn't check all the failures in the analyzer results. They were spread out across the results and I didn't realize. Will address tomorrow.

annevk
annevk reviewed Jul 1, 2025
View reviewed changes
Source/WebKit/WebProcess/Inspector/WebInspectorClient.cpp Outdated
Comment on lines 113 to 114
if (page && page->protectedInspector())
page->protectedInspector()->bringToFront();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (page && page->protectedInspector())
page->protectedInspector()->bringToFront();
if (!page)
return;
if (RefPtr inspector = page->inspector();
inspector->bringToFront();

Is what we did elsewhere for cases like this. You should definitely never need a protected member just for a boolean output.

@jelee53
Adopt smart pointers in WebFullScreenManagerProxy.cpp, WebInspectorCl…
51d2c6a 
…ient.cpp, WebInspectorInternal.cpp

https://bugs.webkit.org/show_bug.cgi?id=295243
rdar://154710862

Reviewed by NOBODY (OOPS!).

Smart pointer adoption as per the static analyzer.

* Source/WebKit/SaferCPPExpectations/UncheckedCallArgsCheckerExpectations:
* Source/WebKit/SaferCPPExpectations/UncountedCallArgsCheckerExpectations:
* Source/WebKit/UIProcess/WebFullScreenManagerProxy.cpp:
(WebKit::WebFullScreenManagerProxy::isFullScreen):
* Source/WebKit/WebProcess/Inspector/WebInspectorClient.cpp:
(WebKit::WebInspectorClient::~WebInspectorClient):
(WebKit::WebInspectorClient::openLocalFrontend):
(WebKit::WebInspectorClient::bringFrontendToFront):
(WebKit::WebInspectorClient::didResizeMainFrame):
(WebKit::WebInspectorClient::showPaintRect):
(WebKit::WebInspectorClient::elementSelectionChanged):
(WebKit::WebInspectorClient::timelineRecordingChanged):
(WebKit::WebInspectorClient::setDeveloperPreferenceOverride):
* Source/WebKit/WebProcess/Inspector/WebInspectorInternal.cpp:
(WebKit::WebInspector::~WebInspector):
(WebKit::WebInspector::openLocalInspectorFrontend):
(WebKit::WebInspector::closeFrontendConnection):
(WebKit::WebInspector::bringToFront):
(WebKit::WebInspector::showMainResourceForFrame):
(WebKit::WebInspector::elementSelectionChanged):
(WebKit::WebInspector::timelineRecordingChanged):
(WebKit::WebInspector::setDeveloperPreferenceOverride):
(WebKit::WebInspector::setEmulatedConditions):
(WebKit::WebInspector::canAttachWindow):
(WebKit::WebInspector::updateDockingAvailability):
* Source/WebKit/WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::protectedInspector):
(WebKit::WebPage::protectedInspectorUI):
(WebKit::WebPage::protectedRemoteInspectorUI):
* Source/WebKit/WebProcess/WebPage/WebPage.h:
@jelee53 jelee53 force-pushed the eng/Adopt-smart-pointers-in-WebFullScreenManagerProxy-cpp-WebInspectorClient-cpp-WebInspectorInternal-cpp branch from ee580ee to 51d2c6a Compare July 1, 2025 09:22
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jul 1, 2025 •
edited
Loading

EWS run on current version of this PR (hash 51d2c6a)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style loading 🛠 ios loading 🛠 mac loading 🛠 wpe ⏳ 🛠 win
loading 🛠 ios-sim ✅ 🛠 mac-AS-debug loading 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl loading 🧪 ios-wk2 loading 🧪 api-mac loading 🧪 api-wpe
loading 🧪 ios-wk2-wpt loading 🧪 mac-wk1 loading 🛠 wpe-cairo
loading 🧪 api-ios loading 🧪 mac-wk2 ⏳ 🛠 gtk
loading 🛠 vision ⏳ 🧪 mac-AS-debug-wk2 ⏳ 🧪 gtk-wk2
loading 🛠 vision-sim loading 🧪 mac-wk2-stress ⏳ 🧪 api-gtk
⏳ 🧪 vision-wk2 loading 🧪 mac-intel-wk2 ⏳ 🛠 playstation
loading 🛠 tv loading 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
⏳ 🛠 watch-sim

jelee53
jelee53 commented Jul 1, 2025
View reviewed changes
Source/WebKit/WebProcess/Inspector/WebInspectorClient.cpp
@@ -103,22 +103,28 @@ void WebInspectorClient::frontendCountChanged(unsigned count)
Inspector::FrontendChannel* WebInspectorClient::openLocalFrontend(InspectorController* controller)
{
if (RefPtr page = m_page.get())
page->inspector()->openLocalInspectorFrontend();
page->protectedInspector()->openLocalInspectorFrontend();
Copy link
Contributor Author

@jelee53 jelee53 Jul 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm keeping around the protectedInspector() and other protected getters just in case others might need them. I can remove though if we think just one usage in the code doesn't require a helper.

Thoughts? @annevk @pvollan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annevk annevk annevk left review comments

@pvollan pvollan Awaiting requested review from pvollan

@dcrousso dcrousso Awaiting requested review from dcrousso dcrousso will be requested when the pull request is marked ready for review dcrousso is a code owner

@patrickangle patrickangle Awaiting requested review from patrickangle patrickangle will be requested when the pull request is marked ready for review patrickangle is a code owner

@cdumez cdumez Awaiting requested review from cdumez cdumez will be requested when the pull request is marked ready for review cdumez is a code owner

Assignees

@jelee53 jelee53

Labels
WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore).
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jelee53 @webkit-early-warning-system @webkit-ews-buildbot @annevk