Bug 157706 – Update the list of elements attachShadow is allowed

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

The spec has updated the list of element on which attachShadow is allowed.
We should update our implementation accordingly to avoid the future compatibility issues.

[Ryosuke Niwa]

*** Bug 157928 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

Created attachment 280320 [details]
Patch

[WebKit Commit Bot]

Attachment 280320 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Document.cpp:1129:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Created attachment 280507 [details]
Fixed GTK/Windows builds

[WebKit Commit Bot]

Attachment 280507 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Document.cpp:1129:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Created attachment 280509 [details]
Another build fix attempt

[WebKit Commit Bot]

Attachment 280509 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Document.cpp:1129:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 280509 [details]
Another build fix attempt

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

Did you change all the headers that you touched to pragma once? Would be nice to do that.

> Source/WebCore/dom/Document.cpp:1124
> +    for (unsigned i = 0; i < localName.length(); i++) {
> +        if (isASCIIUpper(localName[i]))
> +            return CustomElementNameValidationStatus::ContainsUpperCase;
> +        if (localName[i] == '-')
> +            containsHyphen = true;
> +    }

More efficient not to call length() over and over again and index into the string over and over again.

Could use a couple local variables, or alternatively something like the StringView::codeUnits() iterator.

> Source/WebCore/dom/Document.cpp:1138
> +#if ENABLE(MATHML)
> +        || localName == MathMLNames::annotation_xmlTag.localName()
> +#endif

Seems like we might want to check this even if MathML was not turned on.

> Source/WebCore/dom/Element.cpp:1714
> +    static NeverDestroyed<HashSet<AtomicString>> tagNames;
> +    if (tagNames.get().isEmpty()) {
> +        const AtomicString nameList[] = {
> +            articleTag.localName(),
> +            asideTag.localName(),
> +            blockquoteTag.localName(),
> +            bodyTag.localName(),
> +            divTag.localName(),
> +            footerTag.localName(),
> +            h1Tag.localName(),
> +            h2Tag.localName(),
> +            h3Tag.localName(),
> +            h4Tag.localName(),
> +            h5Tag.localName(),
> +            h6Tag.localName(),
> +            headerTag.localName(),
> +            navTag.localName(),
> +            pTag.localName(),
> +            sectionTag.localName(),
> +            spanTag.localName()
> +        };
> +        for (auto& name : nameList)
> +            tagNames.get().add(name);
> +    }

This is not the best idiom, because we end up checking if the set is empty every time.

Also, we don’t want an array of reference counted local names. Instead we want an array of tags, ideally something that can be initialized without any code running.

Something like this:

    static NeverDestroyed<HashSet<AtomicString>> tagNames = [] {
        HashSet<AtomicString> set;
        const HTMLQualifiedName& const tagList[] = {
            articleTag,
            ...
        };
        for (auto& tag : tagList)
            set.add(tag());
        return set;
    };

> Source/WebCore/dom/Element.cpp:1717
> +    if (element.namespaceURI() != xhtmlNamespaceURI)
> +        return false;

I believe it’s more efficient to just check is<HTMLElement>, and I am pretty sure it accomplishes the same thing.

> Source/WebCore/dom/Element.cpp:1719
> +    return tagNames.get().contains(element.localName()) || Document::validateCustomElementName(element.localName()) == CustomElementNameValidationStatus::Valid;

I suggest putting element.localName() into a local variable.

[Darin Adler]

(In reply to comment #8)
>         const HTMLQualifiedName& const tagList[] = {

Actually I think this:

    static const HTMLQualifiedName& const tagList[] = {

[Ryosuke Niwa]

Committed r201739: <http://trac.webkit.org/changeset/201739>

[Radar WebKit Bug Importer]

<rdar://problem/26666315>