Bug 221357

Summary: Avoid an ancestor walk in HTMLFormControlElement::computeWillValidate()
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: New BugsAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, changseok, cmarcelo, esprehn+autocc, ews-watchlist, gyuyoung.kim, kangil.han, mifenton, rniwa, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch rniwa: review+

Description Simon Fraser (smfr) 2021-02-03 13:28:19 PST
Avoid an ancestor walk in HTMLFormControlElement::computeWillValidate()
Comment 1 Simon Fraser (smfr) 2021-02-03 13:29:35 PST
Created attachment 419180 [details]
Patch
Comment 2 Ryosuke Niwa 2021-02-03 14:01:20 PST
Comment on attachment 419180 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419180&action=review

This is fine but another approach is to create a new node flag like we do for SelfOrAncestorHasDirAuto and set this bit whenever an ancestor has datalist.

> Source/WebCore/dom/Document.h:1331
> +    void addDisabledFieldsetElement() { ++m_disabledFieldsetElementsCount; }

Can we avoid changing the unrelated code?

> Source/WebCore/dom/Document.h:1334
> +    bool hasDataListElements() const { return m_dataListElementCount; }

We should have this in TreeScope instead.
Otherwise, we're gonna have a bad time inside a ShadowRoot.

> Source/WebCore/dom/Element.cpp:2220
> +    auto* page = document().page();
> +    if (page) {

Please define page inside if.

> Source/WebCore/html/HTMLDataListElement.cpp:61
> +    if (parentOfInsertedTree.isInTreeScope())

This isn't right. We're checking that the inserted ancestor's new parent is in a tree scope.
It doesn't mean it's in the document.

Make the change above to move the function to TreeScope and this will be correct
but it's better to check insertionType.treeScopeChanged instead even in that case.

> Source/WebCore/html/HTMLDataListElement.cpp:70
> +    if (oldParentOfRemovedTree.isInTreeScope())
> +        document().decrementDataListElementCount();

Ditto.

> Source/WebCore/html/HTMLDataListElement.cpp:76
> +void HTMLDataListElement::didMoveToNewDocument(Document& oldDocument, Document& newDocument)
> +{

This function override won't be needed once we made all the changes I suggest above.
Comment 3 Simon Fraser (smfr) 2021-02-03 17:56:19 PST
Created attachment 419212 [details]
Patch
Comment 4 Ryosuke Niwa 2021-02-03 18:37:21 PST
Comment on attachment 419212 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419212&action=review

> Source/WebCore/ChangeLog:13
> +        * dom/Document.h:
> +        (WebCore::Document::addDisabledFieldsetElement):

Update these change log entries?

> Source/WebCore/html/HTMLDataListElement.cpp:64
> +    if (insertionType.treeScopeChanged)
> +        treeScope().incrementDataListElementCount();

We can do this before calling HTMLElement::insertedIntoAncestor.

> Source/WebCore/page/PointerLockController.cpp:106
> -void PointerLockController::elementRemoved(Element& element)
> +void PointerLockController::elementWasRemoved(Element& element)

Or maybe didRemoveElement?
Comment 5 Simon Fraser (smfr) 2021-02-03 20:54:30 PST
Created attachment 419225 [details]
Patch
Comment 6 Simon Fraser (smfr) 2021-02-03 21:21:32 PST
https://trac.webkit.org/changeset/272358/webkit
Comment 7 Radar WebKit Bug Importer 2021-02-03 21:22:13 PST
<rdar://problem/73964453>