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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=218298

Attachment 412550: Patch

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




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

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
ChildChangeType.

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?