Adopt more smart pointers in RenderElement #19451
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cdumez
added
the
Layout and Rendering
|
EWS run on previous version of this PR (hash 065db4c) |
rniwa
reviewed
Oct 23, 2023
Source/WebCore/page/EventHandler.cpp
Outdated
| @@ -1706,7 +1700,7 @@ std::optional<Cursor> EventHandler::selectCursor(const HitTestResult& result, bo | |||
| #if ENABLE(CURSOR_VISIBILITY) | |||
| void EventHandler::startAutoHideCursorTimer() | |||
| { | |||
| Page* page = m_frame.page(); | |||
| Page* page = m_frame->page(); | |||
There was a problem hiding this comment.
Why not use checkedPage here as well?
There was a problem hiding this comment.
The changes to EventHandler are not meant to be exhaustive. This patch's focus is RenderElement. I just made some changes to EventHandler store it in a CheckedPtr/CheckedRef and changing m_frame ended up making the change large.
Looking at this particular function though, you can see the page is only used to query a setting. I figure this was safe so I didn't opportunistically use CheckedPtr for the page here.
rniwa
reviewed
Oct 23, 2023
Source/WebCore/page/EventHandler.cpp
Outdated
| @@ -1728,11 +1722,11 @@ void EventHandler::cancelAutoHideCursorTimer() | |||
|
|
|||
| void EventHandler::autoHideCursorTimerFired() | |||
| { | |||
| auto* view = m_frame.view(); | |||
| auto* view = m_frame->view(); | |||
There was a problem hiding this comment.
It only called view->isActive() so it seemed excessive?
rniwa
reviewed
Oct 23, 2023
Source/WebCore/page/EventHandler.cpp
Outdated
| MonotonicTime savedLastHandledUserGestureTimestamp; | ||
| bool savedUserDidInteractWithPage = topDocument ? topDocument->userDidInteractWithPage() : false; | ||
|
|
||
| if (m_frame.document()) | ||
| savedLastHandledUserGestureTimestamp = m_frame.document()->lastHandledUserGestureTimestamp(); | ||
| if (auto* document = frame->document()) |
There was a problem hiding this comment.
We only call a trivial getter on the document so this seemed excessive.
Also note that there are still plenty of potentially unsafe pointer usage in this file. The focus on this patch is RenderElement, not EventHandler.
rniwa
reviewed
Oct 23, 2023
Source/WebCore/page/EventHandler.cpp
Outdated
| @@ -4308,10 +4302,10 @@ bool EventHandler::handleDrag(const MouseEventWithHitTestResults& event, CheckDr | |||
| // FIXME: Consider doing this earlier in this function as the earliest point we're sure it would be safe to drop an old drag. | |||
| invalidateDataTransfer(); | |||
|
|
|||
| if (!m_frame.document()) | |||
| if (!frame->document()) | |||
There was a problem hiding this comment.
Maybe we can do:
RefPtr document = frame->document();
if (document)
return false;
dragState().dataTransfer = DataTransfer::createForDrag(*document);
webkit-ews-buildbot
added
the
merging-blocked
rniwa
reviewed
Oct 23, 2023
Source/WebCore/page/EventHandler.h
Outdated
| @@ -136,6 +137,10 @@ class EventHandler { | |||
| explicit EventHandler(LocalFrame&); | |||
| ~EventHandler(); | |||
|
|
|||
| // For CheckedPtr / CheckedRef use. | |||
| void incrementPtrCount() const; | |||
There was a problem hiding this comment.
I think we should just make EventHandler inherit from CanMakeCheckedPtr unless this is absolutely necessary for correctness, in which case the reason should be stated in the commit message.
There was a problem hiding this comment.
I think this goes against @darinadler 's recommendation on my earlier PR?
The EventHandler is completely owned by the Frame via a UniqueRef so we don't need to increase the size of EventHandler unnecessarily. I also made EventHandler::m_frame be a CheckedRef so that the lifetime safety is bi-directional.
I don't mind either way. I merely went this way to be consistent with my earlier PR where I think Darin suggested this (the review comments were not 100% clear to me so I may have misunderstood as well).
rniwa
reviewed
Oct 23, 2023
| @@ -734,7 +734,7 @@ static bool frameHasPlatformWidget(const LocalFrame& frame) | |||
| PlatformMouseEvent EventHandler::currentPlatformMouseEvent() const | |||
| { | |||
| NSView *windowView = nil; | |||
| if (Page* page = m_frame.page()) | |||
| if (Page* page = m_frame->page()) | |||
rniwa
reviewed
Oct 23, 2023
| @@ -338,7 +338,7 @@ static bool findViewInSubviews(NSView *superview, NSView *target) | |||
| if (!mouseDownView) { | |||
| return nil; | |||
| } | |||
| auto* topFrameView = m_frame.view(); | |||
| auto* topFrameView = m_frame->view(); | |||
rniwa
reviewed
Oct 23, 2023
| @@ -518,7 +518,7 @@ static void selfRetainingNSScrollViewScrollWheel(NSScrollView *self, SEL selecto | |||
|
|
|||
| void EventHandler::mouseDown(NSEvent *event, NSEvent *correspondingPressureEvent) | |||
| { | |||
| auto* v = m_frame.view(); | |||
| auto* v = m_frame->view(); | |||
rniwa
reviewed
Oct 23, 2023
| @@ -535,7 +535,7 @@ static void selfRetainingNSScrollViewScrollWheel(NSScrollView *self, SEL selecto | |||
|
|
|||
| void EventHandler::mouseDragged(NSEvent *event, NSEvent *correspondingPressureEvent) | |||
| { | |||
| auto* v = m_frame.view(); | |||
| auto* v = m_frame->view(); | |||
rniwa
reviewed
Oct 23, 2023
| @@ -549,7 +549,7 @@ static void selfRetainingNSScrollViewScrollWheel(NSScrollView *self, SEL selecto | |||
|
|
|||
| void EventHandler::mouseUp(NSEvent *event, NSEvent *correspondingPressureEvent) | |||
| { | |||
| auto* v = m_frame.view(); | |||
| auto* v = m_frame->view(); | |||
rniwa
reviewed
Oct 23, 2023
| @@ -586,7 +586,7 @@ static void selfRetainingNSScrollViewScrollWheel(NSScrollView *self, SEL selecto | |||
| */ | |||
| void EventHandler::sendFakeEventsAfterWidgetTracking(NSEvent *initiatingEvent) | |||
| { | |||
| auto* view = m_frame.view(); | |||
| auto* view = m_frame->view(); | |||
| if (!view) | |||
rniwa
reviewed
Oct 23, 2023
| if (!page) | ||
| return; | ||
|
|
||
| auto* view = m_frame.view(); | ||
| auto* view = m_frame->view(); |
cdumez
force-pushed
the
263542_RenderElement_smart_pointer
branch
from
065db4c
to
cbc9a86
Compare
|
EWS run on previous version of this PR (hash cbc9a86)
|
rniwa
approved these changes
Oct 23, 2023
cdumez
force-pushed
the
263542_RenderElement_smart_pointer
branch
from
cbc9a86
to
80e0df0
Compare
|
EWS run on previous version of this PR (hash 80e0df0)
|
cdumez
force-pushed
the
263542_RenderElement_smart_pointer
branch
from
80e0df0
to
73eae54
Compare
|
EWS run on current version of this PR (hash 73eae54) |
cdumez
added
the
unsafe-merge-queue
https://bugs.webkit.org/show_bug.cgi?id=263542 Reviewed by Ryosuke Niwa. Adopt more smart pointers in RenderElement. I also made some changes to EventHandler so that we can store it in a CheckedPtr / CheckedRef. I made it forward its checked pointer count to its associated frame since it is owned by the frame. As extra safety, I made EventHandler::m_frame be a CheckedRef<Frame> which required me to update all uses of m_frame in this file. That said, the changes in EventHandler are NOT meant to be exhaustive. * Source/WebCore/dom/Document.cpp: (WebCore::Document::protectedView const): * Source/WebCore/page/EventHandler.cpp: (WebCore::EventHandler::incrementPtrCount const): (WebCore::EventHandler::decrementPtrCount const): * Source/WebCore/page/EventHandler.h: * Source/WebCore/page/LocalFrame.cpp: (WebCore::LocalFrame::checkedEventHandler): (WebCore::LocalFrame::checkedEventHandler const): (WebCore::Document::protectedView const): Deleted. * Source/WebCore/page/LocalFrame.h: * Source/WebCore/rendering/RenderElement.cpp: (WebCore::RenderElement::createFor): (WebCore::RenderElement::updateFillImages): (WebCore::RenderElement::updateShapeImage): (WebCore::RenderElement::repaintBeforeStyleChange): (WebCore::RenderElement::initializeStyle): (WebCore::RenderElement::setStyle): (WebCore::RenderElement::didAttachChild): (WebCore::RenderElement::attachRendererInternal): (WebCore::RenderElement::detachRendererInternal): (WebCore::findNextLayer): (WebCore::layerNextSiblingRespectingTopLayer): (WebCore::addLayers): (WebCore::RenderElement::removeLayers): (WebCore::RenderElement::moveLayers): (WebCore::RenderElement::layerParent const): (WebCore::RenderElement::propagateStyleToAnonymousChildren): (WebCore::RenderElement::styleWillChange): (WebCore::RenderElement::styleDidChange): (WebCore::RenderElement::insertedIntoTree): (WebCore::RenderElement::willBeRemovedFromTree): (WebCore::RenderElement::clearSubtreeLayoutRootIfNeeded const): (WebCore::RenderElement::willBeDestroyed): * Source/WebCore/rendering/RenderElement.h: * Source/WebCore/rendering/RenderObject.h: (WebCore::RenderObject::protectedFrame const): * Source/WebCore/rendering/RenderObjectInlines.h: (WebCore::RenderObject::protectedDocument const): * Source/WebCore/rendering/style/NinePieceImage.h: (WebCore::NinePieceImage::protectedImage const): * Source/WebCore/rendering/style/RenderStyle.h: * Source/WebCore/rendering/style/RenderStyleInlines.h: (WebCore::RenderStyle::protectedBackgroundLayers const): (WebCore::RenderStyle::protectedMaskLayers const): (WebCore::RenderStyle::protectedShapeOutside const): * Source/WebCore/rendering/style/ShapeValue.h: (WebCore::ShapeValue::protectedImage const): Canonical link: https://commits.webkit.org/269712@main
webkit-commit-queue
force-pushed
the
263542_RenderElement_smart_pointer
branch
from
73eae54
to
ea4f8e5
Compare
|
Committed 269712@main (ea4f8e5): https://commits.webkit.org/269712@main Reviewed commits have been landed. Closing PR #19451 and removing active labels. |
webkit-commit-queue
removed
the
unsafe-merge-queue
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ea4f8e5
73eae54