Adopt more smart pointers in dom/Text #46876

jelee53 · 2025-06-17T23:20:03Z

`dcd1706`

Adopt more smart pointers in dom/Text
https://bugs.webkit.org/show_bug.cgi?id=294640
rdar://153684504 (Adopt more smart pointers in dom/Text (294640))

Reviewed by Ryosuke Niwa.

Smart pointer adoption as per the static analyzer.

* Source/WebCore/SaferCPPExpectations/UncheckedLocalVarsCheckerExpectations:
* Source/WebCore/SaferCPPExpectations/UncountedLocalVarsCheckerExpectations:
* Source/WebCore/dom/Text.cpp:
(WebCore::Text::wholeText const):
* Source/WebCore/dom/TextNodeTraversal.cpp:
(WebCore::TextNodeTraversal::appendContents):
(WebCore::TextNodeTraversal::childTextContent):
* Source/WebCore/dom/TextNodeTraversal.h:
(WebCore::TextNodeTraversal::firstTextWithinTemplate):
(WebCore::TextNodeTraversal::traverseNextTextTemplate):

Canonical link: https://commits.webkit.org/296685@main

20a3947

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	⏳ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 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~~
✅ 🛠 🧪 unsafe-merge	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2025-06-17T23:20:11Z

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

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 win
✅ 🧪 bindings	⏳ 🛠 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 · 2025-06-17T23:24:05Z

Source/ThirdParty/libwebrtc/libwebrtc.xcodeproj/project.pbxproj

@@ -24290,6 +24290,9 @@
 		FB39D0701200ED9200088E69 /* Project object */ = {
 			isa = PBXProject;
 			attributes = {
+				KnownAssetTags = (


Trying to figure out how to force push a new commit that overwrites this...

Please ignore this for now.

Done. Updated commit should remove this.

webkit-early-warning-system · 2025-06-17T23:28:42Z

EWS run on previous version of this PR (hash 0a81702)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	~~🛠 ios~~	✅ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 win
~~🧪 bindings~~	⏳ 🛠 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

webkit-early-warning-system · 2025-06-17T23:29:53Z

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

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	⏳ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 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

RupinMittal

Looks mostly good, a few small comments.

RupinMittal · 2025-06-23T23:38:23Z

Source/WebCore/dom/Text.cpp

-    const Text* endText = latestLogicallyAdjacentTextNode(this);
-    ASSERT(endText);
-    const Node* onePastEndText = TextNodeTraversal::nextSibling(*endText);
+    RefPtr startText = const_cast<Text*>(earliestLogicallyAdjacentTextNode(this));


I believe we don't need the const_cast<Text*> here. You should be able to just do:

RefPtr startText = earliestLogicallyAdjacentTextNode(this);

Source/WebCore/dom/Text.cpp

RupinMittal · 2025-06-24T00:17:52Z

Source/WebCore/dom/Text.cpp

-    ASSERT(endText);
-    const Node* onePastEndText = TextNodeTraversal::nextSibling(*endText);
+    RefPtr startText = const_cast<Text*>(earliestLogicallyAdjacentTextNode(this));
+    RefPtr endText = const_cast<Text*>(latestLogicallyAdjacentTextNode(this));


Done! Thanks for pointing this out Rupin!

RupinMittal · 2025-06-24T00:18:22Z

Source/WebCore/dom/Text.cpp

-    const Node* onePastEndText = TextNodeTraversal::nextSibling(*endText);
+    RefPtr startText = const_cast<Text*>(earliestLogicallyAdjacentTextNode(this));
+    RefPtr endText = const_cast<Text*>(latestLogicallyAdjacentTextNode(this));
+    RefPtr onePastEndText = const_cast<Text*>(TextNodeTraversal::nextSibling(*endText));


Maybe use:

RefPtr<const Node> onePastEndText = TextNodeTraversal::nextSibling(*endText);

I believe RefPtr would deduce as RefPtr<const Node> without the const_cast. No need to be explicit about it.

RupinMittal · 2025-06-24T00:18:34Z

Source/WebCore/dom/Text.cpp


    StringBuilder result;
-    for (const Text* text = startText; text != onePastEndText; text = TextNodeTraversal::nextSibling(*text))
+    for (RefPtr text = startText; text != onePastEndText; text = const_cast<Text*>(TextNodeTraversal::nextSibling(*text)))


Could use RefPtr<const Text> and remove the const_cast.

We definitely should avoid const_cast as much as possible.

cdumez · 2025-06-24T03:19:48Z

Adopt more smart pointers in dom/Text
https://bugs.webkit.org/show_bug.cgi?id=294640
Include a Radar link (OOPS!).

Please drop the OOPS line or put the radar link if you have one (not mandatory).

cdumez · 2025-06-24T03:20:35Z

When starting out, I'd recommend staying out of dom/ as this is performance sensitive code. This PR will have to be A/B tested on Speedometer.

webkit-early-warning-system · 2025-06-24T18:28:23Z

EWS run on previous version of this PR (hash 8c691e4)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	⏳ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 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

RupinMittal

Almost there, but as Chris mentioned, this will have to be A/B tested before landing.

RupinMittal · 2025-06-24T21:42:53Z

Source/WebCore/dom/Text.cpp

    ASSERT(endText);
-    const Node* onePastEndText = TextNodeTraversal::nextSibling(*endText);
+    RefPtr<const Text> onePastEndText = TextNodeTraversal::nextSibling(*endText);


Let's keep using Node here. So RefPtr<const Node>.

Oops I missed that it was using Node originally. Thanks for catching that Rupin!

RupinMittal · 2025-06-24T21:46:06Z

Source/WebCore/dom/Text.cpp


    StringBuilder result;
-    for (const Text* text = startText; text != onePastEndText; text = TextNodeTraversal::nextSibling(*text))
+    for (RefPtr<const Text> text = startText; text != onePastEndText; text = TextNodeTraversal::nextSibling(*text))


Since startText is RefPtr<const Text> from above, we don't need to specify the <const Text> part again. We can just use RefPtr text here.

Ah I see. When we create RefPtr text variable., RefPtr is automatically created with the same type as StartText (aka a constText as the typeParameter). Good point! This has been fixed and thanks Rupin!

webkit-early-warning-system · 2025-06-24T22:00:29Z

EWS run on current version of this PR (hash 20a3947)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	⏳ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 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~~
✅ 🛠 🧪 unsafe-merge	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-commit-queue · 2025-06-26T21:44:40Z

@jelee53 does not have committer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/metadata/contributors.json.

If you do have committer permissions, please ensure that your GitHub username is added to contributors.json.

Rejecting 20a3947 from merge queue.

https://bugs.webkit.org/show_bug.cgi?id=294640 rdar://153684504 (Adopt more smart pointers in dom/Text (294640)) Reviewed by Ryosuke Niwa. Smart pointer adoption as per the static analyzer. * Source/WebCore/SaferCPPExpectations/UncheckedLocalVarsCheckerExpectations: * Source/WebCore/SaferCPPExpectations/UncountedLocalVarsCheckerExpectations: * Source/WebCore/dom/Text.cpp: (WebCore::Text::wholeText const): * Source/WebCore/dom/TextNodeTraversal.cpp: (WebCore::TextNodeTraversal::appendContents): (WebCore::TextNodeTraversal::childTextContent): * Source/WebCore/dom/TextNodeTraversal.h: (WebCore::TextNodeTraversal::firstTextWithinTemplate): (WebCore::TextNodeTraversal::traverseNextTextTemplate): Canonical link: https://commits.webkit.org/296685@main

webkit-commit-queue · 2025-06-26T21:48:22Z

Committed 296685@main (dcd1706): https://commits.webkit.org/296685@main

Reviewed commits have been landed. Closing PR #46876 and removing active labels.

jelee53 self-assigned this Jun 17, 2025

jelee53 added the New Bugs Unclassified bugs are placed in this component until the correct component can be determined. label Jun 17, 2025

jelee53 commented Jun 17, 2025

View reviewed changes

jelee53 force-pushed the eng/Adopt-more-smart-pointers-in-dom-Text branch from af255e6 to 0a81702 Compare June 17, 2025 23:28

jelee53 force-pushed the eng/Adopt-more-smart-pointers-in-dom-Text branch from 0a81702 to d0244c2 Compare June 17, 2025 23:29

jelee53 requested a review from RupinMittal June 17, 2025 23:57

RupinMittal reviewed Jun 23, 2025

View reviewed changes

jelee53 marked this pull request as ready for review June 24, 2025 00:02

jelee53 requested review from rniwa and cdumez as code owners June 24, 2025 00:02

RupinMittal reviewed Jun 24, 2025

View reviewed changes

jelee53 force-pushed the eng/Adopt-more-smart-pointers-in-dom-Text branch from d0244c2 to 8c691e4 Compare June 24, 2025 18:28

jelee53 requested a review from pvollan June 24, 2025 20:31

RupinMittal reviewed Jun 24, 2025

View reviewed changes

jelee53 force-pushed the eng/Adopt-more-smart-pointers-in-dom-Text branch from 8c691e4 to 20a3947 Compare June 24, 2025 22:00

WebKit deleted a comment from jelee53 Jun 25, 2025

rniwa approved these changes Jun 25, 2025

View reviewed changes

jelee53 added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jun 26, 2025

webkit-commit-queue added merging-blocked Applied to prevent a change from being merged and removed unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing labels Jun 26, 2025

pvollan added unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing and removed merging-blocked Applied to prevent a change from being merged labels Jun 26, 2025

webkit-commit-queue force-pushed the eng/Adopt-more-smart-pointers-in-dom-Text branch from 20a3947 to dcd1706 Compare June 26, 2025 21:48

webkit-commit-queue merged commit dcd1706 into WebKit:main Jun 26, 2025

webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jun 26, 2025

Adopt more smart pointers in dom/Text #46876

Adopt more smart pointers in dom/Text #46876

Conversation

jelee53 commented Jun 17, 2025 • edited by webkit-early-warning-system Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webkit-early-warning-system commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RupinMittal left a comment

Choose a reason for hiding this comment

Uh oh!

RupinMittal Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RupinMittal Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RupinMittal Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdumez commented Jun 24, 2025

Uh oh!

cdumez commented Jun 24, 2025

Uh oh!

webkit-early-warning-system commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RupinMittal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelee53 Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webkit-early-warning-system commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-commit-queue commented Jun 26, 2025

Uh oh!

webkit-commit-queue commented Jun 26, 2025

Uh oh!

Uh oh!

jelee53 commented Jun 17, 2025 •

edited by webkit-early-warning-system

Loading

webkit-early-warning-system commented Jun 17, 2025 •

edited

Loading

webkit-early-warning-system commented Jun 17, 2025 •

edited

Loading

webkit-early-warning-system commented Jun 17, 2025 •

edited

Loading

RupinMittal Jun 23, 2025 •

edited

Loading

RupinMittal Jun 24, 2025 •

edited

Loading

RupinMittal Jun 24, 2025 •

edited

Loading

webkit-early-warning-system commented Jun 24, 2025 •

edited

Loading

jelee53 Jun 24, 2025 •

edited

Loading

webkit-early-warning-system commented Jun 24, 2025 •

edited

Loading