Bug 218298

Created attachment 412550 [details] Patch

Comment on attachment 412550 [details] 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?

(In reply to Darin Adler from comment #2) > 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. Ah, I see. > 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. In that case, should we make it enum class or keep it as enum? > > Source/WebCore/dom/ContainerNode.h:77 > > enum class ChildChangeSource { Parser, API }; > > Should probably put a base type of bool here? I worried that but I did not touch it. I'll change it in the next patch.

(In reply to Tetsuharu Ohzeki from comment #3) > (In reply to Darin Adler from comment #2) > > 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. > > In that case, should we make it enum class or keep it as enum? I’m conflicted about that.

(In reply to Darin Adler from comment #4) > (In reply to Tetsuharu Ohzeki from comment #3) > > (In reply to Darin Adler from comment #2) > > > 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. > > > > In that case, should we make it enum class or keep it as enum? > > I’m conflicted about that. I'm trying to move ChildChangeType to ChildChange and keep it enum class. it would be a bit redundant like `ChildChange::Type::ElementInserted`. I seem `ChildChange::ElementInserted` is concise at here and we achive to push ChildChangeType into some scope. So I will change to this pattern in the next patch. And then, I also feel that we can move ChildChangeSource to ChildChange with keeping it as enum class. Darin, how do you think about these?

I like ChildChange::Type::ElementInserted. Even though it’s long, the separators actually make it easier to read.

Okay. Thank you.

(In reply to Tetsuharu Ohzeki [UTC+9] from comment #5) > And then, I also feel that we can move ChildChangeSource to ChildChange with > keeping it as enum class. Yes!

Created attachment 412575 [details] Patch

Comment on attachment 412575 [details] 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.

Created attachment 412619 [details] Patch

Committed r269161: <https://trac.webkit.org/changeset/269161> All reviewed patches have been landed. Closing bug and clearing flags on attachment 412619 [details].

<rdar://problem/70820615>