[webkit-reviews] review granted: [Bug 218298] Make WebCore::ContainerNode::ChildChangeType enum class : [Attachment 412550] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Oct 28 12:51:18 PDT 2020

Darin Adler <darin at apple.com> has granted Tetsuharu Ohzeki
<tetsuharu.ohzeki at gmail.com>'s request for review:
Bug 218298: Make WebCore::ContainerNode::ChildChangeType enum class

Attachment 412550: Patch


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

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

I’m going to say review+ even though I am not *sure* this is an improvement.

> Source/WebCore/dom/ContainerNode.h:76
> +    enum class ChildChangeType: uint8_t { ElementInserted, ElementRemoved,
TextInserted, TextRemoved, TextChanged, AllChildrenRemoved,
NonContentsChildRemoved, NonContentsChildInserted, AllChildrenReplaced };

Not sure this is an improvement. I understand that we like the idea of using
enum class everywhere, but this makes all the code wordier and it’s not
*obvious* to me that it’s better.

We know it’s better when the actual enumeration values can have shorter more
straightforward names because the enumeration’s name will always be cited just
before them. But here we were not able to do that.

Maybe this enumeration should be a in the ChildChange structure, with the name
"Type" instead of next to the ChildChange structure with the name

Note that we can set the base type to uint8_t even without switching from enum
to enum class.

> Source/WebCore/dom/ContainerNode.h:77
>      enum class ChildChangeSource { Parser, API };

Should probably put a base type of bool here?

More information about the webkit-reviews mailing list