Never make nodes which do not recalc style style-dirty.

Comments, processing instructions, attr, and doctype nodes should never
have style, are never marked dirty, never runs through style recalc and
marked clean, and should not have style dirty initially.

Attr and doctype nodes will never run through AttachLayoutTree since
they can not be descendants of the root element, but since PI and
comments can, we need to say that it's OK that they don't need
attachment for AttachLayoutTree.

Also, don't mark the ancestor chain with ChildNeedsStyleRecalc when
inserting such nodes into a tree.

This is fixing the TODO introduced in 492730.

Bug: 492730, 868810
Change-Id: I4470ce11a94e1390c2c6a9b7d1420c0e28844ade
Reviewed-on: https://chromium-review.googlesource.com/1186729Reviewed-by: Anders Ruud <andruud@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585819}

third_party/blink/renderer/core/dom/comment.h

@@ -27,7 +27,7 @@
 
 namespace blink {
 
-class Comment final : public CharacterData {
+class CORE_EXPORT Comment final : public CharacterData {
   DEFINE_WRAPPERTYPEINFO();
 
  public:
@@ -39,6 +39,7 @@ class Comment final : public CharacterData {
   String nodeName() const override;
   NodeType getNodeType() const override;
   Node* Clone(Document&, CloneChildrenFlag) const override;
+  void DetachLayoutTree(const AttachContext&) final {}
 };
 
 DEFINE_NODE_TYPE_CASTS(Comment, getNodeType() == Node::kCommentNode);

third_party/blink/renderer/core/dom/container_node.cc

@@ -951,11 +951,34 @@ void ContainerNode::NotifyNodeRemoved(Node& root) {
   }
 }
 
+#if DCHECK_IS_ON()
+namespace {
+
+bool AttachedAllowedWhenAttaching(Node* node) {
+  return node->getNodeType() == Node::kCommentNode ||
+         node->getNodeType() == Node::kProcessingInstructionNode;
+}
+
+bool ChildAttachedAllowedWhenAttachingChildren(ContainerNode* node) {
+  if (node->IsShadowRoot())
+    return true;
+  if (node->IsV0InsertionPoint())
+    return true;
+  if (IsHTMLSlotElement(node))
+    return true;
+  if (IsShadowHost(node))
+    return true;
+  return false;
+}
+
+}  // namespace
+#endif
+
 DISABLE_CFI_PERF
 void ContainerNode::AttachLayoutTree(AttachContext& context) {
   for (Node* child = firstChild(); child; child = child->nextSibling()) {
 #if DCHECK_IS_ON()
-    DCHECK(child->NeedsAttach() ||
+    DCHECK(child->NeedsAttach() || AttachedAllowedWhenAttaching(child) ||
            ChildAttachedAllowedWhenAttachingChildren(this));
 #endif
     if (child->NeedsAttach())
@@ -982,11 +1005,10 @@ void ContainerNode::ChildrenChanged(const ChildrenChange& change) {
   GetDocument().IncDOMTreeVersion();
   GetDocument().NotifyChangeChildren(*this);
   InvalidateNodeListCachesInAncestors(nullptr, nullptr, &change);
-  if (change.IsChildInsertion()) {
-    if (!ChildNeedsStyleRecalc()) {
-      SetChildNeedsStyleRecalc();
-      MarkAncestorsWithChildNeedsStyleRecalc();
-    }
+  if (!ChildNeedsStyleRecalc() && change.IsChildInsertion() &&
+      change.sibling_changed->NeedsStyleRecalc()) {
+    SetChildNeedsStyleRecalc();
+    MarkAncestorsWithChildNeedsStyleRecalc();
   }
 }
 
@@ -1621,22 +1643,4 @@ NodeListsNodeData& ContainerNode::EnsureNodeLists() {
   return EnsureRareData().EnsureNodeLists();
 }
 
-#if DCHECK_IS_ON()
-bool ChildAttachedAllowedWhenAttachingChildren(ContainerNode* node) {
-  if (node->IsShadowRoot())
-    return true;
-
-  if (node->IsV0InsertionPoint())
-    return true;
-
-  if (IsHTMLSlotElement(node))
-    return true;
-
-  if (IsShadowHost(node))
-    return true;
-
-  return false;
-}
-#endif
-
 }  // namespace blink

third_party/blink/renderer/core/dom/container_node.h

@@ -456,10 +456,6 @@ class CORE_EXPORT ContainerNode : public Node {
   TraceWrapperMember<Node> last_child_;
 };
 
-#if DCHECK_IS_ON()
-bool ChildAttachedAllowedWhenAttachingChildren(ContainerNode*);
-#endif
-
 WILL_NOT_BE_EAGERLY_TRACED_CLASS(ContainerNode);
 
 DEFINE_NODE_TYPE_CASTS(ContainerNode, IsContainerNode());

third_party/blink/renderer/core/dom/document.cc

@@ -2124,13 +2124,6 @@ void Document::PropagateStyleToViewport() {
 #if DCHECK_IS_ON()
 static void AssertLayoutTreeUpdated(Node& root) {
   for (Node& node : NodeTraversal::InclusiveDescendantsOf(root)) {
-    // We leave some nodes with dirty bits in the tree because they don't
-    // matter like Comment and ProcessingInstruction nodes.
-    // TODO(esprehn): Don't even mark those nodes as needing recalcs in the
-    // first place.
-    if (!node.IsElementNode() && !node.IsTextNode() && !node.IsShadowRoot() &&
-        !node.IsDocumentNode())
-      continue;
     DCHECK(!node.NeedsStyleRecalc());
     DCHECK(!node.ChildNeedsStyleRecalc());
     DCHECK(!node.NeedsReattachLayoutTree());

third_party/blink/renderer/core/dom/document_type.h

@@ -55,6 +55,7 @@ class DocumentType final : public Node {
 
   InsertionNotificationRequest InsertedInto(ContainerNode&) override;
   void RemovedFrom(ContainerNode&) override;
+  void DetachLayoutTree(const AttachContext&) final {}
 
   String name_;
   String public_id_;

third_party/blink/renderer/core/dom/node.cc

@@ -908,6 +908,8 @@ void Node::SetNeedsStyleRecalc(StyleChangeType change_type,
   DCHECK(change_type != kNoStyleChange);
   if (!InActiveDocument())
     return;
+  if (!IsContainerNode() && !IsTextNode())
+    return;
 
   TRACE_EVENT_INSTANT1(
       TRACE_DISABLED_BY_DEFAULT("devtools.timeline.invalidationTracking"),

third_party/blink/renderer/core/dom/node.h

@@ -932,7 +932,7 @@ class CORE_EXPORT Node : public EventTarget {
 
  protected:
   enum ConstructionType {
-    kCreateOther = kDefaultNodeFlags,
+    kCreateOther = kIsFinishedParsingChildrenFlag,
     kCreateText = kDefaultNodeFlags | kIsTextFlag,
     kCreateContainer =
         kDefaultNodeFlags | kChildNeedsStyleRecalcFlag | kIsContainerFlag,

third_party/blink/renderer/core/dom/node_test.cc

@@ -6,9 +6,11 @@
 
 #include "third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.h"
 #include "third_party/blink/renderer/core/css/resolver/style_resolver.h"
+#include "third_party/blink/renderer/core/dom/comment.h"
 #include "third_party/blink/renderer/core/dom/element.h"
 #include "third_party/blink/renderer/core/dom/flat_tree_traversal.h"
 #include "third_party/blink/renderer/core/dom/layout_tree_builder.h"
+#include "third_party/blink/renderer/core/dom/processing_instruction.h"
 #include "third_party/blink/renderer/core/dom/shadow_root.h"
 #include "third_party/blink/renderer/core/dom/shadow_root_init.h"
 #include "third_party/blink/renderer/core/editing/testing/editing_test_base.h"
@@ -315,4 +317,21 @@ TEST_F(NodeTest, HasMediaControlAncestor_MediaControls) {
   EXPECT_TRUE(InitializeUserAgentShadowTree(node)->HasMediaControlAncestor());
 }
 
+TEST_F(NodeTest, appendChildProcessingInstructionNoStyleRecalc) {
+  GetDocument().View()->UpdateAllLifecyclePhases();
+  EXPECT_FALSE(GetDocument().ChildNeedsStyleRecalc());
+  ProcessingInstruction* pi =
+      ProcessingInstruction::Create(GetDocument(), "A", "B");
+  GetDocument().body()->appendChild(pi, ASSERT_NO_EXCEPTION);
+  EXPECT_FALSE(GetDocument().ChildNeedsStyleRecalc());
+}
+
+TEST_F(NodeTest, appendChildCommentNoStyleRecalc) {
+  GetDocument().View()->UpdateAllLifecyclePhases();
+  EXPECT_FALSE(GetDocument().ChildNeedsStyleRecalc());
+  Comment* comment = Comment::Create(GetDocument(), "comment");
+  GetDocument().body()->appendChild(comment, ASSERT_NO_EXCEPTION);
+  EXPECT_FALSE(GetDocument().ChildNeedsStyleRecalc());
+}
+
 }  // namespace blink

third_party/blink/renderer/core/dom/processing_instruction.h

@@ -32,8 +32,8 @@ namespace blink {
 class StyleSheet;
 class EventListener;
 
-class ProcessingInstruction final : public CharacterData,
-                                    private ResourceClient {
+class CORE_EXPORT ProcessingInstruction final : public CharacterData,
+                                                private ResourceClient {
   DEFINE_WRAPPERTYPEINFO();
   USING_GARBAGE_COLLECTED_MIXIN(ProcessingInstruction);
 
@@ -80,6 +80,7 @@ class ProcessingInstruction final : public CharacterData,
 
   InsertionNotificationRequest InsertedInto(ContainerNode&) override;
   void RemovedFrom(ContainerNode&) override;
+  void DetachLayoutTree(const AttachContext&) final {}
 
   bool CheckStyleSheet(String& href, String& charset);
   void Process(const String& href, const String& charset);