Skip to content
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

Add more smart pointers to highlight code #29656

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add more smart pointers to highlight code #29656

wants to merge 1 commit into from

Conversation

abigailfox
Copy link
Contributor

@abigailfox abigailfox commented Jun 8, 2024 •
edited by webkit-early-warning-system

5b29a84

Add more smart pointers to highlight code
https://bugs.webkit.org/show_bug.cgi?id=275295
rdar://129454373

Reviewed by NOBODY (OOPS!).

* Source/WebCore/Modules/highlight/AppHighlightStorage.cpp:
(WebCore::findNodeStartingAtPathComponentIndex):
(WebCore::findNode):
(WebCore::makeNodePath):
(WebCore::AppHighlightStorage::attemptToRestoreHighlightAndScroll):
* Source/WebCore/Modules/highlight/Highlight.cpp:
(WebCore::Highlight::clearFromSetLike):
* Source/WebCore/Modules/highlight/HighlightRegistry.cpp:
(WebCore::HighlightRegistry::setHighlightVisibility):
(WebCore::HighlightRegistry::addAnnotationHighlightWithRange):
* Source/WebCore/dom/Document.cpp:
(WebCore::Document::protectedAppHighlightRegistry):
* Source/WebCore/dom/Document.h:

5b29a84

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 wincairo-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
✅ 🛠 watch
✅ 🛠 watch-sim

@abigailfox
Add more smart pointers to highlight code
5b29a84 
https://bugs.webkit.org/show_bug.cgi?id=275295
rdar://129454373

Reviewed by NOBODY (OOPS!).

* Source/WebCore/Modules/highlight/AppHighlightStorage.cpp:
(WebCore::findNodeStartingAtPathComponentIndex):
(WebCore::findNode):
(WebCore::makeNodePath):
(WebCore::AppHighlightStorage::attemptToRestoreHighlightAndScroll):
* Source/WebCore/Modules/highlight/Highlight.cpp:
(WebCore::Highlight::clearFromSetLike):
* Source/WebCore/Modules/highlight/HighlightRegistry.cpp:
(WebCore::HighlightRegistry::setHighlightVisibility):
(WebCore::HighlightRegistry::addAnnotationHighlightWithRange):
* Source/WebCore/dom/Document.cpp:
(WebCore::Document::protectedAppHighlightRegistry):
* Source/WebCore/dom/Document.h:
@abigailfox abigailfox self-assigned this Jun 8, 2024
@abigailfox abigailfox added the WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). label Jun 8, 2024
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash 5b29a84)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 wincairo-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
✅ 🛠 watch
✅ 🛠 watch-sim

@whsieh whsieh requested a review from megangardner June 8, 2024 22:36
nt1m
nt1m reviewed Jun 9, 2024
View reviewed changes
Source/WebCore/Modules/highlight/AppHighlightStorage.cpp
@@ -88,10 +88,11 @@ static std::pair<RefPtr<Node>, size_t> findNodeStartingAtPathComponentIndex(cons

static RefPtr<Node> findNode(const AppHighlightRangeData::NodePath& path, Document& document)
{
if (path.isEmpty() || !document.body())
Ref documentRef = document;
Copy link
Member

@nt1m nt1m Jun 9, 2024 •
edited

Choose a reason for hiding this comment

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

I think this isn't needed since the only call site protects it: https://searchfox.org/wubkat/rev/1e4577ba8de434bc8942b01baf6b1d65ba967ab1/Source/WebCore/Modules/highlight/AppHighlightStorage.cpp#259

In any case, having a protector is against the guidelines I think since it should be done at the call site level.

@abigailfox abigailfox requested a review from ddkilzer June 12, 2024 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nt1m nt1m nt1m left review comments

@rniwa rniwa Awaiting requested review from rniwa rniwa is a code owner

@cdumez cdumez Awaiting requested review from cdumez cdumez is a code owner

@megangardner megangardner Awaiting requested review from megangardner

@ddkilzer ddkilzer Awaiting requested review from ddkilzer

Assignees

@abigailfox abigailfox

Labels
WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit).
Projects
None yet
Milestone
No milestone
3 participants
@abigailfox @webkit-early-warning-system @nt1m