[Webkit-unassigned] [Bug 218298] Make WebCore::ContainerNode::ChildChangeType enum class

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=218298

--- Comment #10 from Darin Adler <darin at apple.com> ---
Comment on attachment 412575
  --> https://bugs.webkit.org/attachment.cgi?id=412575
Patch

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

> Source/WebCore/dom/ContainerNode.cpp:85
> +    if (source == ContainerNode::ChildChange::Source::API) {

The ContainerNode prefix isn’t needed here. We are inside a ContainerNode member function here.

> Source/WebCore/dom/ContainerNode.cpp:93
> +        ASSERT(source == ContainerNode::ChildChange::Source::Parser);

Ditto.

> Source/WebCore/dom/ContainerNode.cpp:133
> +    if (source == ContainerNode::ChildChange::Source::API) {

Ditto.

> Source/WebCore/dom/ContainerNode.cpp:140
> +    if (source == ContainerNode::ChildChange::Source::Parser) {

Ditto.

> Source/WebCore/dom/ContainerNode.cpp:169
> +        change.type = is<Element>(childToRemove) ? ChildChange::Type::ElementRemoved : (is<Text>(childToRemove) ? ChildChange::Type::TextRemoved : ChildChange::Type::NonContentsChildRemoved);

Here we do it all on one line.

> Source/WebCore/dom/ContainerNode.cpp:208
> +            child.isElementNode() ?
> +                ContainerNode::ChildChange::Type::ElementInserted :
> +                (child.isTextNode() ?
> +                    ContainerNode::ChildChange::Type::TextInserted :
> +                    ContainerNode::ChildChange::Type::NonContentsChildInserted),

Here we break it into multiple lines.

Not really elegant to have that difference.

> Source/WebCore/dom/ContainerNode.h:78
> +        enum class Type: uint8_t { ElementInserted, ElementRemoved, TextInserted, TextRemoved, TextChanged, AllChildrenRemoved, NonContentsChildRemoved, NonContentsChildInserted, AllChildrenReplaced };
> +        enum class Source: bool { Parser, API };

WebKit’s normal formatting puts a space before the ":".

> Source/WebCore/dom/Element.cpp:2627
> +        SiblingCheckType checkType = change.type == ChildChange::Type::ElementRemoved ? SiblingElementRemoved : Other;

I think this is better with "auto" instead of SiblingCheckType.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20201028/aec3693c/attachment-0001.htm>