CSS: Document Invalidate and do less work for slotted.

Invalidate is the core of the recursive invalidation algorithm, add
some commentary to make it more obvious what is going on.

In the process of writing the doc, I thought I found a bug in that we
could underinvalidate with ::slotted when WholeSubtreeInvalid is
true. However the style recalculation due to WholeSubtreeInvalid will
ensure that even if we underinvalidate we will recalculate anyway.

So we can ignore the slotted logic when WholeSubtreeInvalid is true.

Change-Id: Ifbca3c5b52b66e0ef9da54d1e04888055d1748e9
Reviewed-on: https://chromium-review.googlesource.com/1090594
Commit-Queue: Fergal Daly <fergal@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565966}

third_party/blink/renderer/core/css/invalidation/style_invalidator.cc

@@ -246,8 +246,14 @@ void StyleInvalidator::InvalidateChildren(Element& element) {
 
 void StyleInvalidator::Invalidate(Element& element, SiblingData& sibling_data) {
   sibling_data.Advance();
+  // Preserves the current stack of pending invalidations and other state and
+  // restores it when this method returns.
   RecursionCheckpoint checkpoint(this);
 
+  // If we have already entered a subtree that is going to be entirely
+  // recalculated then there is no need to test against current invalidation
+  // sets or to continue to accumulate new invalidation sets as we descend the
+  // tree.
   if (!WholeSubtreeInvalid()) {
     if (element.GetStyleChangeType() >= kSubtreeStyleChange) {
       SetWholeSubtreeInvalid();
@@ -258,8 +264,21 @@ void StyleInvalidator::Invalidate(Element& element, SiblingData& sibling_data) {
     }
     if (UNLIKELY(element.NeedsStyleInvalidation()))
       PushInvalidationSetsForContainerNode(element, sibling_data);
+
+    // When a slot element is invalidated, the slotted elements are also
+    // invalidated by HTMLSlotElement::DidRecalcStyle. So if WholeSubtreeInvalid
+    // is true, they will be included even though they are not part of the
+    // subtree. It's not necessary to fully recalc style for the slotted
+    // elements in that case as they just need to pick up changed inherited
+    // styles but we do it. If we ever stop doing that then this code and the
+    // PushInvalidationSetsForContainerNode above need to move out of the
+    // if-block.
+    if (InvalidatesSlotted() && IsHTMLSlotElement(element))
+      InvalidateSlotDistributedElements(ToHTMLSlotElement(element));
   }
 
+  // If we have invalidation sets that could apply to descendants or we know
+  // there are invalidation sets to be found in the descendants then we descend.
   if (HasInvalidationSets() || element.ChildNeedsStyleInvalidation())
     InvalidateChildren(element);
 
@@ -267,8 +286,6 @@ void StyleInvalidator::Invalidate(Element& element, SiblingData& sibling_data) {
     element.SetNeedsStyleRecalc(kSubtreeStyleChange,
                                 StyleChangeReasonForTracing::Create(
                                     StyleChangeReason::kStyleInvalidator));
-  if (InvalidatesSlotted() && IsHTMLSlotElement(element))
-    InvalidateSlotDistributedElements(ToHTMLSlotElement(element));
 
   element.ClearChildNeedsStyleInvalidation();
   element.ClearNeedsStyleInvalidation();

third_party/blink/renderer/core/html/html_slot_element.cc

@@ -505,6 +505,10 @@ void HTMLSlotElement::DidRecalcStyle(StyleRecalcChange change) {
       ToElement(node)->RecalcStyleForReattach();
       continue;
     }
+    // We only need to pick up changes for inherited style, we do not actually
+    // need to match rules against this element but we do that for
+    // simplicity. If we ever stop doing this then we need to update
+    // StyleInvalidator::Invalidate as described in the comment there.
     node->SetNeedsStyleRecalc(
         kLocalStyleChange,
         StyleChangeReasonForTracing::Create(