Store nonAttachedStyle on NodeLayoutData instead of on HashMap in Document.

third_party/WebKit/Source/core/dom/Document.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  *           (C) 2006 Alexey Proskuryakov (ap@webkit.org)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2011, 2012 Apple Inc. All
  * rights reserved.
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved.
  * (http://www.torchmobile.com/)
  * Copyright (C) 2008, 2009, 2011, 2012 Google Inc. All rights reserved.
@@ skipping 1415 matching lines @@
 
       return body;
     }
 
     return documentElement();
   }
 
   return body();
 }
 
-// We use HashMap::set over HashMap::add here as we want to
-// replace the ComputedStyle but not the Node if the Node is
-// already present.
-void Document::AddNonAttachedStyle(const Node& node,
-                                   RefPtr<ComputedStyle> computed_style) {
-  DCHECK(node.IsElementNode() || node.IsTextNode());
-  non_attached_style_.Set(&node, computed_style);
-}
-
-ComputedStyle* Document::GetNonAttachedStyle(const Node& node) const {
-  return non_attached_style_.at(&node);
-}
-
 /*
  * Performs three operations:
  *  1. Convert control characters to spaces
  *  2. Trim leading and trailing spaces
  *  3. Collapse internal whitespace.
  */
 template <typename CharacterType>
 static inline String CanonicalizedTitle(Document* document,
                                         const String& title) {
   unsigned length = title.length();
@@ skipping 661 matching lines @@
     }
     if (document_element->NeedsReattachLayoutTree() ||
         document_element->ChildNeedsReattachLayoutTree()) {
       TRACE_EVENT0("blink,blink_style", "Document::rebuildLayoutTree");
       document_element->RebuildLayoutTree();
     }
   }
 
   View()->RecalcOverflowAfterStyleChange();
 
-  // Only retain the HashMap for the duration of StyleRecalc and
-  // LayoutTreeConstruction.
-  non_attached_style_.Clear();

[rune, 2017/04/19 20:34:52]

Do we retain the ComputedStyle on each node now, even after we have created the
layout objects?

Also, does that mean that if we later detach a subtree (layout tree), we will
still retain all descendant ComputedStyles in a display:none subtree? If so, is
that intentional?

[nainar, 2017/04/19 23:43:41]

For the needs of this patch I should delete the computedStyle once we get the
info in rebuildLayoutTree but in a future patch I do want to retain
ComputedStyle for even display:non elements. 

I don't know what is the best place to clear the non attached ComputedStyle.
Can't do it in Element::rebuildLayoutTree because of the SharedStyleFinder which
may get called in LayoutTreeBuilder. And clearing it in LayoutTreeBuilder::style
would mean that elements that don't get a LayoutObject will still retain this
information. 

[rune, 2017/04/20 08:56:39]

So, if we have a subtree of N elements which is initially rendered, and make the
root of that subtree display:none and never display it again, should we retain
all N ComputedStyles for the lifetime of the subtree?

Later, it would make sense to unify non-attached style with the computed_style_
in ElementRareData?
It looks like we currently only store non-attached style for the root of a
layout tree which needs rebuilding (I know this is not the end goal). Can't you
clear the non-attached style after calling ReattachLayoutTree() in
Element::RebuildLayoutTree()?

[nainar, 2017/04/21 00:51:30]

We shouldn't be retaining the nonAttachedStyle for those nodes at this stage. I
have the same intent to unify it with the ComputedStyle stored on
ElementRareData. Iterating over all descendants clearing their nonAttachedStyle
when the root is set to display:none sounds liek a new perf regression to me
though. Am I wrong?
Done. 

[rune, 2017/04/21 07:41:46]

No, this was more of a future concern.
Detach will traverse over displayed descendants of the root and the rare-data
computed style is already cleared in DetachLayoutTree or AttachLayoutTree today:

https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/core/d...

[nainar, 2017/04/26 02:55:16]

Done. 

Had to change Node::setNonAttachedStyle() to incorporate the ability to set
nonAttachedStyle to null

[rune, 2017/04/26 08:47:28]

Sorry for being unclear, but I think the patch set at the time I lgtm'ed was
ready for landing (modulo any SetNonAttachedStyle() you had to change).

I was just thinking ahead about a future CL where we unify non-attached style
with non-layout-object-computed-style. I think the non-attached style is only
set for the root node of a subtree being re-attached as RecalcStyle is currently
skipping descendants when we get a kReattach change.

   ClearChildNeedsStyleRecalc();
   ClearChildNeedsReattachLayoutTree();
 
   resolver.ClearStyleSharingList();
 
   DCHECK(!NeedsStyleRecalc());
   DCHECK(!ChildNeedsStyleRecalc());
   DCHECK(!NeedsReattachLayoutTree());
   DCHECK(!ChildNeedsReattachLayoutTree());
   DCHECK(InStyleRecalc());
   DCHECK_EQ(GetStyleResolver(), &resolver);
-  DCHECK(non_attached_style_.IsEmpty());
   lifecycle_.AdvanceTo(DocumentLifecycle::kStyleClean);
   if (should_record_stats) {
     TRACE_EVENT_END2(
         "blink,blink_style", "Document::updateStyle", "resolverAccessCount",
         GetStyleEngine().StyleForElementCount() - initial_element_count,
         "counters", GetStyleEngine().Stats()->ToTracedValue());
   } else {
     TRACE_EVENT_END1(
         "blink,blink_style", "Document::updateStyle", "resolverAccessCount",
         GetStyleEngine().StyleForElementCount() - initial_element_count);
@@ skipping 4431 matching lines @@
   visitor->Trace(user_action_elements_);
   visitor->Trace(svg_extensions_);
   visitor->Trace(timeline_);
   visitor->Trace(compositor_pending_animations_);
   visitor->Trace(context_document_);
   visitor->Trace(canvas_font_cache_);
   visitor->Trace(intersection_observer_controller_);
   visitor->Trace(snap_coordinator_);
   visitor->Trace(resize_observer_controller_);
   visitor->Trace(property_registry_);
-  visitor->Trace(non_attached_style_);
   visitor->Trace(network_state_observer_);
   Supplementable<Document>::Trace(visitor);
   TreeScope::Trace(visitor);
   ContainerNode::Trace(visitor);
   ExecutionContext::Trace(visitor);
   SecurityContext::Trace(visitor);
   SynchronousMutationNotifier::Trace(visitor);
 }
 
 void Document::RecordDeferredLoadReason(WouldLoadReason reason) {
@@ skipping 38 matching lines @@
 }
 
 void showLiveDocumentInstances() {
   WeakDocumentSet& set = liveDocumentSet();
   fprintf(stderr, "There are %u documents currently alive:\n", set.size());
   for (blink::Document* document : set)
     fprintf(stderr, "- Document %p URL: %s\n", document,
             document->Url().GetString().Utf8().Data());
 }
 #endif