Skip to content

Use more smart pointers for dynamicDowncast<>in SVG code#27385

Merged
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
rwlbuis:eng/Use-more-smart-pointers-for-dynamicDowncastin-SVG-code
Apr 18, 2024
Merged

Use more smart pointers for dynamicDowncast<>in SVG code#27385
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
rwlbuis:eng/Use-more-smart-pointers-for-dynamicDowncastin-SVG-code

Conversation

@rwlbuis
Copy link
Contributor

@rwlbuis rwlbuis commented Apr 17, 2024 •
edited by webkit-early-warning-system
Loading

7196d1f

Use more smart pointers for dynamicDowncast<>in SVG code
https://bugs.webkit.org/show_bug.cgi?id=272817

Reviewed by Chris Dumez.

Use more smart pointers for dynamicDowncast<>in SVG code.

* Source/WebCore/rendering/svg/RenderSVGText.cpp:
(WebCore::findPreviousAndNextAttributes):
* Source/WebCore/rendering/svg/RenderSVGTransformableContainer.cpp:
(WebCore::RenderSVGTransformableContainer::additionalContainerTranslation const):
* Source/WebCore/rendering/svg/SVGBoundingBoxComputation.cpp:
(WebCore::SVGBoundingBoxComputation::handleRootOrContainer const):
* Source/WebCore/rendering/svg/SVGContainerLayout.cpp:
(WebCore::SVGContainerLayout::layoutChildren):
(WebCore::SVGContainerLayout::positionChildrenRelativeToContainer):
* Source/WebCore/rendering/svg/SVGRenderSupport.cpp:
(WebCore::SVGRenderSupport::checkForSVGRepaintDuringLayout):
(WebCore::updateObjectBoundingBox):
(WebCore::SVGRenderSupport::computeContainerBoundingBoxes):
(WebCore::SVGRenderSupport::computeContainerStrokeBoundingBox):
(WebCore::layoutSizeOfNearestViewportChanged):
(WebCore::SVGRenderSupport::layoutChildren):
(WebCore::isPointInCSSClippingArea):
(WebCore::SVGRenderSupport::clipContextToCSSClippingArea):
(WebCore::SVGRenderSupport::calculateApproximateStrokeBoundingBox):
* Source/WebCore/rendering/svg/SVGRenderTreeAsText.cpp:
(WebCore::writeSVGPaintingFeatures):
(WebCore::writeResources):
* Source/WebCore/rendering/svg/SVGTextMetricsBuilder.cpp:
(WebCore::SVGTextMetricsBuilder::walkTree):
* Source/WebCore/rendering/svg/SVGTextQuery.cpp:
(WebCore::flowBoxForRenderer):
* Source/WebCore/rendering/svg/legacy/LegacyRenderSVGModelObject.cpp:
(WebCore::getElementCTM):
* Source/WebCore/rendering/svg/legacy/LegacyRenderSVGResourceClipper.cpp:
(WebCore::LegacyRenderSVGResourceClipper::pathOnlyClipping):
* Source/WebCore/rendering/svg/legacy/LegacyRenderSVGTransformableContainer.cpp:
(WebCore::LegacyRenderSVGTransformableContainer::calculateLocalTransform):
* Source/WebCore/svg/SVGFEComponentTransferElement.cpp:
(WebCore::SVGFEComponentTransferElement::setFilterEffectAttributeFromChild):
* Source/WebCore/svg/SVGFontFaceSrcElement.cpp:
(WebCore::SVGFontFaceSrcElement::createSrcValue const):
* Source/WebCore/svg/SVGGraphicsElement.cpp:
(WebCore::SVGGraphicsElement::invalidateResourceImageBuffersIfNeeded):

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

7cd1ccd

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

@rwlbuis rwlbuis self-assigned this Apr 17, 2024
@rwlbuis rwlbuis added the Layout and Rendering For bugs with layout and rendering of Web pages. label Apr 17, 2024
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Apr 17, 2024 •
edited
Loading

EWS run on previous version of this PR (hash 4fb3246)

Details

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

@rwlbuis rwlbuis requested a review from cdumez April 17, 2024 17:53
cdumez
cdumez approved these changes Apr 17, 2024
View reviewed changes
Copy link
Contributor

@cdumez cdumez left a comment

Choose a reason for hiding this comment

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

We need to be careful when adopting smart pointers to actually follow the guidelines. Otherwise, we'll make the performance worse without actually improving security.

If you find it difficult, maybe we should wait for the static analyzer bot to be ready.

Source/WebCore/rendering/svg/RenderSVGText.cpp Outdated
Copy link
Contributor

@cdumez cdumez Apr 17, 2024

Choose a reason for hiding this comment

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

Not strictly needed since layoutAttributes() is a trivial inline function. If this is hot code, we could consider not making this change.

Copy link
Contributor Author

@rwlbuis rwlbuis Apr 18, 2024

Choose a reason for hiding this comment

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

Ah, sorry, did not check layoutAttributes properly.

Source/WebCore/rendering/svg/SVGContainerLayout.cpp Outdated
Copy link
Contributor

@cdumez cdumez Apr 17, 2024

Choose a reason for hiding this comment

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

Not strictly needed, isLayoutSizeChanged() is a trivial inline getter.

Copy link
Contributor Author

@rwlbuis rwlbuis Apr 18, 2024

Choose a reason for hiding this comment

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

Fixed.

Source/WebCore/rendering/svg/SVGContainerLayout.cpp Outdated
Copy link
Contributor

@cdumez cdumez Apr 17, 2024

Choose a reason for hiding this comment

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

ditto.

Copy link
Contributor Author

@rwlbuis rwlbuis Apr 18, 2024

Choose a reason for hiding this comment

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

Fixed.

Source/WebCore/rendering/svg/SVGRenderSupport.cpp Outdated
Copy link
Contributor

@cdumez cdumez Apr 17, 2024

Choose a reason for hiding this comment

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

ditto.

Copy link
Contributor Author

@rwlbuis rwlbuis Apr 18, 2024

Choose a reason for hiding this comment

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

Done.

Source/WebCore/rendering/svg/SVGRenderTreeAsText.cpp Outdated
Copy link
Contributor

@cdumez cdumez Apr 17, 2024

Choose a reason for hiding this comment

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

ditto.

Copy link
Contributor Author

@rwlbuis rwlbuis Apr 18, 2024

Choose a reason for hiding this comment

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

Done.

Source/WebCore/rendering/svg/SVGRenderTreeAsText.cpp Outdated
Copy link
Contributor

@cdumez cdumez Apr 17, 2024

Choose a reason for hiding this comment

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

Not strictly needed since the call site is supposed to have a Ref/RefPtr to svgElement on the stack. It's not this function's job to keep parameters alive.

Same comment applies elsewhere in this function.

Copy link
Contributor Author

@rwlbuis rwlbuis Apr 18, 2024

Choose a reason for hiding this comment

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

Got it, fixed.

Source/WebCore/rendering/svg/SVGRenderingContext.cpp Outdated
Copy link
Contributor

@cdumez cdumez Apr 17, 2024

Choose a reason for hiding this comment

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

Not needed.

Copy link
Contributor Author

@rwlbuis rwlbuis Apr 18, 2024

Choose a reason for hiding this comment

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

Removed this part.

Source/WebCore/svg/SVGLinearGradientElement.cpp Outdated
Copy link
Contributor

@cdumez cdumez Apr 17, 2024

Choose a reason for hiding this comment

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

gradientElement.releaseNonNull() to avoid refcounting churn. Or keep it as a raw pointer. There was nothing wrong with this code.

Copy link
Contributor Author

@rwlbuis rwlbuis Apr 18, 2024

Choose a reason for hiding this comment

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

Kept as-is.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 17, 2024
@rwlbuis rwlbuis removed the merging-blocked Applied to prevent a change from being merged label Apr 18, 2024
@rwlbuis rwlbuis force-pushed the eng/Use-more-smart-pointers-for-dynamicDowncastin-SVG-code branch from 4fb3246 to 7cd1ccd Compare April 18, 2024 08:57
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Apr 18, 2024 •
edited
Loading

EWS run on current version of this PR (hash 7cd1ccd)

Details

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

@rwlbuis rwlbuis added the merge-queue Applied to send a pull request to merge-queue label Apr 18, 2024
@rwlbuis
Use more smart pointers for dynamicDowncast<>in SVG code
7196d1f 
https://bugs.webkit.org/show_bug.cgi?id=272817

Reviewed by Chris Dumez.

Use more smart pointers for dynamicDowncast<>in SVG code.

* Source/WebCore/rendering/svg/RenderSVGText.cpp:
(WebCore::findPreviousAndNextAttributes):
* Source/WebCore/rendering/svg/RenderSVGTransformableContainer.cpp:
(WebCore::RenderSVGTransformableContainer::additionalContainerTranslation const):
* Source/WebCore/rendering/svg/SVGBoundingBoxComputation.cpp:
(WebCore::SVGBoundingBoxComputation::handleRootOrContainer const):
* Source/WebCore/rendering/svg/SVGContainerLayout.cpp:
(WebCore::SVGContainerLayout::layoutChildren):
(WebCore::SVGContainerLayout::positionChildrenRelativeToContainer):
* Source/WebCore/rendering/svg/SVGRenderSupport.cpp:
(WebCore::SVGRenderSupport::checkForSVGRepaintDuringLayout):
(WebCore::updateObjectBoundingBox):
(WebCore::SVGRenderSupport::computeContainerBoundingBoxes):
(WebCore::SVGRenderSupport::computeContainerStrokeBoundingBox):
(WebCore::layoutSizeOfNearestViewportChanged):
(WebCore::SVGRenderSupport::layoutChildren):
(WebCore::isPointInCSSClippingArea):
(WebCore::SVGRenderSupport::clipContextToCSSClippingArea):
(WebCore::SVGRenderSupport::calculateApproximateStrokeBoundingBox):
* Source/WebCore/rendering/svg/SVGRenderTreeAsText.cpp:
(WebCore::writeSVGPaintingFeatures):
(WebCore::writeResources):
* Source/WebCore/rendering/svg/SVGTextMetricsBuilder.cpp:
(WebCore::SVGTextMetricsBuilder::walkTree):
* Source/WebCore/rendering/svg/SVGTextQuery.cpp:
(WebCore::flowBoxForRenderer):
* Source/WebCore/rendering/svg/legacy/LegacyRenderSVGModelObject.cpp:
(WebCore::getElementCTM):
* Source/WebCore/rendering/svg/legacy/LegacyRenderSVGResourceClipper.cpp:
(WebCore::LegacyRenderSVGResourceClipper::pathOnlyClipping):
* Source/WebCore/rendering/svg/legacy/LegacyRenderSVGTransformableContainer.cpp:
(WebCore::LegacyRenderSVGTransformableContainer::calculateLocalTransform):
* Source/WebCore/svg/SVGFEComponentTransferElement.cpp:
(WebCore::SVGFEComponentTransferElement::setFilterEffectAttributeFromChild):
* Source/WebCore/svg/SVGFontFaceSrcElement.cpp:
(WebCore::SVGFontFaceSrcElement::createSrcValue const):
* Source/WebCore/svg/SVGGraphicsElement.cpp:
(WebCore::SVGGraphicsElement::invalidateResourceImageBuffersIfNeeded):

Canonical link: https://commits.webkit.org/277661@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Use-more-smart-pointers-for-dynamicDowncastin-SVG-code branch from 7cd1ccd to 7196d1f Compare April 18, 2024 11:45
@webkit-commit-queue
Copy link
Collaborator

webkit-commit-queue commented Apr 18, 2024

Committed 277661@main (7196d1f): https://commits.webkit.org/277661@main

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

@webkit-commit-queue webkit-commit-queue merged commit 7196d1f into WebKit:main Apr 18, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Apr 18, 2024
@rwlbuis rwlbuis deleted the eng/Use-more-smart-pointers-for-dynamicDowncastin-SVG-code branch April 22, 2024 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cdumez cdumez cdumez approved these changes

Assignees

@rwlbuis rwlbuis

Labels

Layout and Rendering For bugs with layout and rendering of Web pages.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@rwlbuis @webkit-early-warning-system @webkit-commit-queue @cdumez @webkit-ews-buildbot

Comments