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
+68 −37

Conversation

jelee53
Copy link
Contributor

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

1698769

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!).

Apply https://github.com/WebKit/WebKit/wiki/Safer-CPP-Guidelines

* 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:

bb0b2a2

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios loading 🛠 mac ✅ 🛠 wpe ⏳ 🛠 win
loading 🛠 ios-sim ✅ 🛠 mac-AS-debug loading 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl loading 🧪 ios-wk2 loading 🧪 api-mac ✅ 🧪 api-wpe
loading 🧪 ios-wk2-wpt loading 🧪 mac-wk1 ✅ 🛠 wpe-cairo
⏳ 🛠 🧪 jsc loading 🧪 api-ios loading 🧪 mac-wk2 ✅ 🛠 gtk
loading 🛠 vision ⏳ 🧪 mac-AS-debug-wk2 loading 🧪 gtk-wk2
loading 🛠 vision-sim loading 🧪 mac-wk2-stress loading 🧪 api-gtk
⏳ 🧪 vision-wk2 loading 🧪 mac-intel-wk2   🛠 playstation
✅ 🛠 tv loading 🛠 mac-safer-cpp
✅ 🛠 tv-sim
loading 🛠 watch
loading 🛠 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 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 previous version of this PR (hash 51d2c6a)

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 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

Copy link
Contributor

Choose a reason for hiding this comment

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

Given it's on page() I think that's warranted, might be worth a quick grep to confirm, but seems likely it's needed elsewhere.

@webkit-ews-buildbot
Copy link
Collaborator

Safer C++ Build #42214 (51d2c6a)

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

annevk
annevk reviewed Jul 1, 2025
View reviewed changes
Source/WebKit/WebProcess/Inspector/WebInspectorClient.cpp Outdated
@@ -189,13 +195,13 @@ void WebInspectorClient::showPaintRect(const FloatRect& rect)

if (!m_paintRectOverlay) {
m_paintRectOverlay = PageOverlay::create(*this, PageOverlay::OverlayType::Document);
page->corePage()->pageOverlayController().installPageOverlay(*m_paintRectOverlay, PageOverlay::FadeMode::DoNotFade);
page->corePage()->pageOverlayController().installPageOverlay(*(m_paintRectOverlay.copyRef()), PageOverlay::FadeMode::DoNotFade);
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that you need m_paintRectOverlay again below I'd write this as:

RefPtr paintRectOverlay = m_paintRectOverlay;
if (!paintRectOverlay)
    paintRectOverlay = ...
    m_paintRectOverlay = paintRectOverlay.copyRef();

...
... paintRectOverlay->layer();

I think that ends up being clearer overall.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made some stylistic updates to this where paintRectOverlay is not assigned until after m_paintRectOverlay is created. Please let me know what you think!

annevk
annevk approved these changes Jul 1, 2025
View reviewed changes
Copy link
Contributor

@annevk annevk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Modulo m_paintRectOverlay nits this looks good to me. (Which I see now are also being flagged by the analyzer as wrong. Oops.)

Source/WebKit/WebProcess/Inspector/WebInspectorClient.cpp Outdated
@@ -80,7 +80,7 @@ WebInspectorClient::~WebInspectorClient()
if (m_paintRectOverlay) {
RefPtr page = m_page.get();
if (page && page->corePage())
page->corePage()->pageOverlayController().uninstallPageOverlay(*m_paintRectOverlay, PageOverlay::FadeMode::Fade);
page->corePage()->pageOverlayController().uninstallPageOverlay(*(m_paintRectOverlay.copyRef()), PageOverlay::FadeMode::Fade);
Copy link
Contributor

Choose a reason for hiding this comment

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

You should obtain a RefPtr to m_paintRectOverlay as part of the conditional above and deref that here.

@jelee53 jelee53 force-pushed the eng/Adopt-smart-pointers-in-WebFullScreenManagerProxy-cpp-WebInspectorClient-cpp-WebInspectorInternal-cpp branch from 51d2c6a to 1698769 Compare July 1, 2025 18:23
@webkit-early-warning-system
Copy link
Collaborator

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

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

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 ❌ 🧪 mac-wk1 ❌ 🛠 wpe-cairo
⏳ 🛠 🧪 jsc ❌ 🧪 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
Adopt smart pointers in WebFullScreenManagerProxy.cpp, WebInspectorCl…
bb0b2a2 
…ient.cpp, WebInspectorInternal.cpp

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

Reviewed by NOBODY (OOPS!).

Apply https://github.com/WebKit/WebKit/wiki/Safer-CPP-Guidelines

* 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 1698769 to bb0b2a2 Compare July 1, 2025 18:39
@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 bb0b2a2)

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

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

@annevk annevk annevk approved these changes

@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