Use dynamicDowncast<T> more in html/ #20711

cdumez · 2023-11-18T22:05:41Z

`ac61808`

Use dynamicDowncast<T> more in html/
https://bugs.webkit.org/show_bug.cgi?id=265092

Reviewed by Jean-Yves Avenard.

Use dynamicDowncast<T> more in html/.

I did a little bit of smart pointer adoption on the lines I
changed too.

* Source/WebCore/html/BaseDateAndTimeInputType.cpp:
(WebCore::BaseDateAndTimeInputType::updateInnerTextValue):
* Source/WebCore/html/CachedHTMLCollectionInlines.h:
(WebCore::nameShouldBeVisibleInDocumentAll):
* Source/WebCore/html/CanvasBase.cpp:
(WebCore:: const):
* Source/WebCore/html/CustomPaintCanvas.cpp:
(WebCore::CustomPaintCanvas::getContext):
* Source/WebCore/html/DOMFormData.cpp:
(WebCore::createFileEntry):
* Source/WebCore/html/FTPDirectoryDocument.cpp:
(WebCore::FTPDirectoryDocumentParser::loadDocumentTemplate):
* Source/WebCore/html/FeaturePolicy.cpp:
(WebCore::isFeaturePolicyAllowedByDocumentAndAllOwners):
* Source/WebCore/html/FileInputType.cpp:
(isType):
* Source/WebCore/html/FormController.cpp:
(WebCore::shouldBeUsedForFormSignature):
* Source/WebCore/html/GenericCachedHTMLCollection.cpp:
(WebCore::GenericCachedHTMLCollection<traversalType>::elementMatches const):
* Source/WebCore/html/HTMLAnchorElement.cpp:
(WebCore::appendServerMapMousePosition):
(WebCore::HTMLAnchorElement::defaultEventHandler):
(WebCore::HTMLAnchorElement::eventType):
(WebCore::isEnterKeyKeydownEvent):
* Source/WebCore/html/HTMLAreaElement.cpp:
(WebCore::HTMLAreaElement::imageElement const):
(WebCore::HTMLAreaElement::setFocus):
* Source/WebCore/html/HTMLAttachmentElement.cpp:
(WebCore::HTMLAttachmentElement::enclosingImageElement const):
* Source/WebCore/html/HTMLAudioElement.h:
(isType):
* Source/WebCore/html/HTMLButtonElement.cpp:
(WebCore::HTMLButtonElement::defaultEventHandler):
* Source/WebCore/html/HTMLCanvasElement.cpp:
(WebCore::HTMLCanvasElement::getContext):
(WebCore::HTMLCanvasElement::didDraw):
(WebCore::HTMLCanvasElement::reset):
(WebCore::HTMLCanvasElement::getImageData):
(WebCore::HTMLCanvasElement::toVideoFrame):
(WebCore::HTMLCanvasElement::setImageBufferAndMarkDirty):
(WebCore::HTMLCanvasElement::clearImageBuffer const):
(WebCore::HTMLCanvasElement::virtualHasPendingActivity const):
* Source/WebCore/html/HTMLCanvasElement.h:
* Source/WebCore/html/HTMLCollection.cpp:
(WebCore::HTMLCollection::updateNamedElementCache const):
* Source/WebCore/html/HTMLDetailsElement.cpp:
(WebCore::DetailsSlotAssignment::slotNameForHostChild const):
* Source/WebCore/html/HTMLDocument.cpp:
(WebCore::HTMLDocument::namedItem):
* Source/WebCore/html/HTMLDocument.h:
(isType):
* Source/WebCore/html/HTMLElement.cpp:
(WebCore::elementAffectsDirectionality):
(WebCore::HTMLElement::editabilityFromContentEditableAttr):
(WebCore::HTMLElement::insertedIntoAncestor):
(WebCore::HTMLElement::setOuterText):
(WebCore::HTMLElement::computeDirectionalityFromText const):
(WebCore::HTMLElement::dirAttributeChanged):
(WebCore::HTMLElement::canBeActuallyDisabled const):
(WebCore::checkPopoverValidity):

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

4b0a8ec

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	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
	✅ 🛠 watch
✅ 🛠 🧪 unsafe-merge	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-11-18T22:05:51Z

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

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	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

jyavenard

with nit/questions

jyavenard · 2023-11-19T22:25:18Z

Source/WebCore/html/HTMLCanvasElement.cpp

-            return std::optional<RenderingContext> { RefPtr<WebGL2RenderingContext> { &downcast<WebGL2RenderingContext>(*m_context) } };
+            if (auto* context = dynamicDowncast<WebGLRenderingContext>(*m_context))
+                return std::optional<RenderingContext> { RefPtr { context } };
+            return std::optional<RenderingContext> { RefPtr { &downcast<WebGL2RenderingContext>(*m_context) } };


I think this should be checkedDowncast even if it was just a debug assert before (considering a lot of the changes add safety check such as going from raw pointers to RefPtr)

Sure, I can make this a checkedDowncast<>().

jyavenard · 2023-11-19T22:27:55Z

Source/WebCore/html/HTMLCanvasElement.cpp

        }
 #endif

-        if (m_context->isWebGPU()) {
+        if (auto* context = dynamicDowncast<GPUCanvasContext>(m_context.get())) {
            if (!isWebGPUType(contextId))


existing, but isWebGPUType test could be moved up. potentially remove one check.

Ok, will look into making this change.

I looked into it a bit more and I think I prefer to leave the code as is because:

It is consistent with the other if checks above in this function

If I move the if (!isWebGPUType(contextId)) earlier, then we won't hit the ASSERT_NOT_REACHED() later in this function if we fail to deal with a particular context type.

jyavenard · 2023-11-19T22:30:17Z

Source/WebCore/html/HTMLCanvasElement.cpp

@@ -584,8 +582,8 @@ void HTMLCanvasElement::reset()
    int w = limitToOnlyHTMLNonNegative(attributeWithoutSynchronization(widthAttr), defaultWidth);
    int h = limitToOnlyHTMLNonNegative(attributeWithoutSynchronization(heightAttr), defaultHeight);

-    if (is<CanvasRenderingContext2D>(m_context))
-        downcast<CanvasRenderingContext2D>(*m_context).reset();
+    if (RefPtr context = dynamicDowncast<CanvasRenderingContext2D>(m_context.get()))


do we need to take a ref here?

this replacement pattern is repeated multiple times, maybe worth a mention in the commit log as it's not just a replacement with dynamicDowncast

I will mention in the commit log. I did a little of smart pointer adoption in the patch too, on the lines I changed. Here we're calling a non-trivial function on the context so we're expected to be holding a local RefPtr/Ref to the context.

webkit-early-warning-system · 2023-11-19T22:51:20Z

EWS run on current version of this PR (hash 4b0a8ec)

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	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
	✅ 🛠 watch
✅ 🛠 🧪 unsafe-merge	✅ 🛠 watch-sim

https://bugs.webkit.org/show_bug.cgi?id=265092 Reviewed by Jean-Yves Avenard. Use dynamicDowncast<T> more in html/. I did a little bit of smart pointer adoption on the lines I changed too. * Source/WebCore/html/BaseDateAndTimeInputType.cpp: (WebCore::BaseDateAndTimeInputType::updateInnerTextValue): * Source/WebCore/html/CachedHTMLCollectionInlines.h: (WebCore::nameShouldBeVisibleInDocumentAll): * Source/WebCore/html/CanvasBase.cpp: (WebCore:: const): * Source/WebCore/html/CustomPaintCanvas.cpp: (WebCore::CustomPaintCanvas::getContext): * Source/WebCore/html/DOMFormData.cpp: (WebCore::createFileEntry): * Source/WebCore/html/FTPDirectoryDocument.cpp: (WebCore::FTPDirectoryDocumentParser::loadDocumentTemplate): * Source/WebCore/html/FeaturePolicy.cpp: (WebCore::isFeaturePolicyAllowedByDocumentAndAllOwners): * Source/WebCore/html/FileInputType.cpp: (isType): * Source/WebCore/html/FormController.cpp: (WebCore::shouldBeUsedForFormSignature): * Source/WebCore/html/GenericCachedHTMLCollection.cpp: (WebCore::GenericCachedHTMLCollection<traversalType>::elementMatches const): * Source/WebCore/html/HTMLAnchorElement.cpp: (WebCore::appendServerMapMousePosition): (WebCore::HTMLAnchorElement::defaultEventHandler): (WebCore::HTMLAnchorElement::eventType): (WebCore::isEnterKeyKeydownEvent): * Source/WebCore/html/HTMLAreaElement.cpp: (WebCore::HTMLAreaElement::imageElement const): (WebCore::HTMLAreaElement::setFocus): * Source/WebCore/html/HTMLAttachmentElement.cpp: (WebCore::HTMLAttachmentElement::enclosingImageElement const): * Source/WebCore/html/HTMLAudioElement.h: (isType): * Source/WebCore/html/HTMLButtonElement.cpp: (WebCore::HTMLButtonElement::defaultEventHandler): * Source/WebCore/html/HTMLCanvasElement.cpp: (WebCore::HTMLCanvasElement::getContext): (WebCore::HTMLCanvasElement::didDraw): (WebCore::HTMLCanvasElement::reset): (WebCore::HTMLCanvasElement::getImageData): (WebCore::HTMLCanvasElement::toVideoFrame): (WebCore::HTMLCanvasElement::setImageBufferAndMarkDirty): (WebCore::HTMLCanvasElement::clearImageBuffer const): (WebCore::HTMLCanvasElement::virtualHasPendingActivity const): * Source/WebCore/html/HTMLCanvasElement.h: * Source/WebCore/html/HTMLCollection.cpp: (WebCore::HTMLCollection::updateNamedElementCache const): * Source/WebCore/html/HTMLDetailsElement.cpp: (WebCore::DetailsSlotAssignment::slotNameForHostChild const): * Source/WebCore/html/HTMLDocument.cpp: (WebCore::HTMLDocument::namedItem): * Source/WebCore/html/HTMLDocument.h: (isType): * Source/WebCore/html/HTMLElement.cpp: (WebCore::elementAffectsDirectionality): (WebCore::HTMLElement::editabilityFromContentEditableAttr): (WebCore::HTMLElement::insertedIntoAncestor): (WebCore::HTMLElement::setOuterText): (WebCore::HTMLElement::computeDirectionalityFromText const): (WebCore::HTMLElement::dirAttributeChanged): (WebCore::HTMLElement::canBeActuallyDisabled const): (WebCore::checkPopoverValidity): Canonical link: https://commits.webkit.org/270960@main

webkit-commit-queue · 2023-11-20T01:42:32Z

Committed 270960@main (ac61808): https://commits.webkit.org/270960@main

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

cdumez requested a review from rniwa as a code owner November 18, 2023 22:05

cdumez self-assigned this Nov 18, 2023

cdumez added the DOM For bugs specific to XML/HTML DOM elements (including parsing). label Nov 18, 2023

cdumez requested review from darinadler and brentfulgham November 19, 2023 18:38

jyavenard approved these changes Nov 19, 2023

View reviewed changes

cdumez force-pushed the 265092_htmL_dynamicDowncast branch from f6e3e58 to 4b0a8ec Compare November 19, 2023 22:51

cdumez added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Nov 20, 2023

webkit-commit-queue force-pushed the 265092_htmL_dynamicDowncast branch from 4b0a8ec to d3aefea Compare November 20, 2023 01:39

webkit-commit-queue force-pushed the 265092_htmL_dynamicDowncast branch from d3aefea to ac61808 Compare November 20, 2023 01:42

webkit-commit-queue merged commit ac61808 into WebKit:main Nov 20, 2023

webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Nov 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use dynamicDowncast<T> more in html/ #20711

Use dynamicDowncast<T> more in html/ #20711

cdumez commented Nov 18, 2023 •

edited by webkit-early-warning-system

Loading

webkit-early-warning-system commented Nov 18, 2023 •

edited

Loading

jyavenard left a comment

jyavenard Nov 19, 2023

cdumez Nov 19, 2023

jyavenard Nov 19, 2023

cdumez Nov 19, 2023

cdumez Nov 19, 2023

jyavenard Nov 19, 2023

cdumez Nov 19, 2023

webkit-early-warning-system commented Nov 19, 2023 •

edited

Loading

webkit-commit-queue commented Nov 20, 2023

Use dynamicDowncast<T> more in html/ #20711

Use dynamicDowncast<T> more in html/ #20711

Conversation

cdumez commented Nov 18, 2023 • edited by webkit-early-warning-system Loading

webkit-early-warning-system commented Nov 18, 2023 • edited Loading

jyavenard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Nov 19, 2023 • edited Loading

webkit-commit-queue commented Nov 20, 2023

cdumez commented Nov 18, 2023 •

edited by webkit-early-warning-system

Loading

webkit-early-warning-system commented Nov 18, 2023 •

edited

Loading

webkit-early-warning-system commented Nov 19, 2023 •

edited

Loading