Skip to content

Commit

Permalink
Move more of style update into StyleEngine.
Browse files Browse the repository at this point in the history 
This is mostly about moving code from Document into StyleEngine.

The other change is relying on the StyleTraversalRoots for deciding if
we need to do style invalidation, recalc, or layout tree rebuilding
instead of the dirty flags on the Document or documentElement(). It is a
simpler way of doing the check since we are no longer marking the
Document with ChildNeedsReattachLayoutTree() and we will not mark the
Document with ChildNeedsStyleRecalc() when FlatTreeStyleRecalc is
enabled.

Bug: 972752
Change-Id: I68a5dd1ffaa4f4f844a7641c143c301ce7417687
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1840832
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#703279}
  • Loading branch information
Rune Lillesveen authored and Commit Bot committed Oct 7, 2019
1 parent cfe7900 commit 9490574
Show file tree
Hide file tree
Showing 6 changed files with 115 additions and 88 deletions.
3 changes: 2 additions & 1 deletion third_party/blink/renderer/core/css/invalidation/style_invalidator_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "third_party/blink/renderer/core/css/invalidation/style_invalidator.h"

#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/blink/renderer/core/css/style_engine.h"
#include "third_party/blink/renderer/core/dom/document.h"
#include "third_party/blink/renderer/core/html/html_element.h"
#include "third_party/blink/renderer/core/testing/dummy_page_holder.h"
Expand Down Expand Up @@ -49,7 +50,7 @@ TEST_F(StyleInvalidatorTest, SkipDisplayNone) {
StyleInvalidator invalidator(pending.GetPendingInvalidationMap());
invalidator.Invalidate(GetDocument(), GetDocument().body());

EXPECT_FALSE(GetDocument().NeedsLayoutTreeUpdate());
EXPECT_FALSE(GetDocument().GetStyleEngine().NeedsStyleRecalc());
}

TEST_F(StyleInvalidatorTest, SkipDisplayNoneClearPendingNth) {
Expand Down
70 changes: 70 additions & 0 deletions third_party/blink/renderer/core/css/style_engine.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,12 @@
#include "third_party/blink/renderer/core/dom/flat_tree_traversal.h"
#include "third_party/blink/renderer/core/dom/layout_tree_builder_traversal.h"
#include "third_party/blink/renderer/core/dom/node_computed_style.h"
#include "third_party/blink/renderer/core/dom/nth_index_cache.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/text.h"
#include "third_party/blink/renderer/core/frame/settings.h"
#include "third_party/blink/renderer/core/html/html_body_element.h"
#include "third_party/blink/renderer/core/html/html_iframe_element.h"
#include "third_party/blink/renderer/core/html/html_link_element.h"
#include "third_party/blink/renderer/core/html/html_slot_element.h"
Expand All @@ -69,13 +71,15 @@
#include "third_party/blink/renderer/core/layout/layout_object.h"
#include "third_party/blink/renderer/core/layout/layout_view.h"
#include "third_party/blink/renderer/core/page/page.h"
#include "third_party/blink/renderer/core/paint/paint_layer.h"
#include "third_party/blink/renderer/core/probe/core_probes.h"
#include "third_party/blink/renderer/core/style/computed_style.h"
#include "third_party/blink/renderer/core/style/style_initial_data.h"
#include "third_party/blink/renderer/core/svg/svg_style_element.h"
#include "third_party/blink/renderer/platform/fonts/font_cache.h"
#include "third_party/blink/renderer/platform/fonts/font_selector.h"
#include "third_party/blink/renderer/platform/heap/heap.h"
#include "third_party/blink/renderer/platform/instrumentation/histogram.h"
#include "third_party/blink/renderer/platform/instrumentation/tracing/trace_event.h"
#include "third_party/blink/renderer/platform/wtf/vector.h"

Expand Down Expand Up @@ -1777,6 +1781,67 @@ void StyleEngine::RebuildLayoutTree() {
in_layout_tree_rebuild_ = false;
}

void StyleEngine::UpdateStyleAndLayoutTree() {
// All of layout tree dirtiness and rebuilding needs to happen on a stable
// flat tree. We have an invariant that all of that happens in this method
// as a result of style recalc and the following layout tree rebuild.
//
// NeedsReattachLayoutTree() marks dirty up the flat tree ancestors. Re-
// slotting on a dirty tree could break ancestor chains and fail to update the
// tree properly.
DCHECK(!NeedsLayoutTreeRebuild());

UpdateViewportStyle();

if (Element* document_element = GetDocument().documentElement()) {
NthIndexCache nth_index_cache(GetDocument());
if (NeedsStyleRecalc()) {
TRACE_EVENT0("blink,blink_style", "Document::recalcStyle");
SCOPED_BLINK_UMA_HISTOGRAM_TIMER_HIGHRES("Style.RecalcTime");
Element* viewport_defining = GetDocument().ViewportDefiningElement();
RecalcStyle();
if (viewport_defining != GetDocument().ViewportDefiningElement())
ViewportDefiningElementDidChange();
}
MarkForWhitespaceReattachment();
if (NeedsLayoutTreeRebuild()) {
TRACE_EVENT0("blink,blink_style", "Document::rebuildLayoutTree");
SCOPED_BLINK_UMA_HISTOGRAM_TIMER_HIGHRES("Style.RebuildLayoutTreeTime");
RebuildLayoutTree();
}
} else {
style_recalc_root_.Clear();
}
ClearWhitespaceReattachSet();
UpdateColorSchemeBackground();
}

void StyleEngine::ViewportDefiningElementDidChange() {
HTMLBodyElement* body = GetDocument().FirstBodyElement();
if (!body || body->NeedsReattachLayoutTree())
return;
LayoutObject* layout_object = body->GetLayoutObject();
if (layout_object && layout_object->IsLayoutBlock()) {
// When the overflow style for documentElement changes to or from visible,
// it changes whether the body element's box should have scrollable overflow
// on its own box or propagated to the viewport. If the body style did not
// need a recalc, this will not be updated as its done as part of setting
// ComputedStyle on the LayoutObject. Force a SetStyle for body when the
// ViewportDefiningElement changes in order to trigger an update of
// HasOverflowClip() and the PaintLayer in StyleDidChange().
layout_object->SetStyle(ComputedStyle::Clone(*layout_object->Style()));
// CompositingReason::kClipsCompositingDescendants depends on the root
// element having a clip-related style. Since style update due to changes of
// viewport-defining element don't end up as a StyleDifference, we need a
// special dirty bit for this situation.
if (layout_object->HasLayer()) {
ToLayoutBoxModelObject(layout_object)
->Layer()
->SetNeedsCompositingReasonsUpdate();
}
}
}

void StyleEngine::UpdateStyleInvalidationRoot(ContainerNode* ancestor,
Node* dirty_node) {
DCHECK(IsMaster());
Expand Down Expand Up @@ -1912,6 +1977,11 @@ void StyleEngine::UpdateViewportStyle() {
}
}

bool StyleEngine::NeedsFullStyleUpdate() const {
return NeedsActiveStyleUpdate() || NeedsWhitespaceReattachment() ||
IsViewportStyleDirty();
}

void StyleEngine::Trace(blink::Visitor* visitor) {
visitor->Trace(document_);
visitor->Trace(injected_user_style_sheets_);
Expand Down
12 changes: 12 additions & 0 deletions third_party/blink/renderer/core/css/style_engine.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,17 @@ class CORE_EXPORT StyleEngine final : public GarbageCollected<StyleEngine>,

scoped_refptr<StyleInitialData> MaybeCreateAndGetInitialData();

bool NeedsStyleInvalidation() const {
return style_invalidation_root_.GetRootNode();
}
bool NeedsStyleRecalc() const { return style_recalc_root_.GetRootNode(); }
bool NeedsLayoutTreeRebuild() const {
return layout_tree_rebuild_root_.GetRootNode();
}
bool NeedsFullStyleUpdate() const;

void UpdateViewportStyle();
void UpdateStyleAndLayoutTree();
void RecalcStyle();
void RebuildLayoutTree();
bool InRebuildLayoutTree() const { return in_layout_tree_rebuild_; }
Expand Down Expand Up @@ -452,6 +462,8 @@ class CORE_EXPORT StyleEngine final : public GarbageCollected<StyleEngine>,
void UpdateColorScheme();
bool SupportsDarkColorScheme();

void ViewportDefiningElementDidChange();

Member<Document> document_;
bool is_master_;

Expand Down
19 changes: 10 additions & 9 deletions third_party/blink/renderer/core/css/style_engine_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1838,10 +1838,10 @@ TEST_F(StyleEngineTest, MarkForWhitespaceReattachment) {
EXPECT_TRUE(GetStyleEngine().NeedsWhitespaceReattachment(d1));
EXPECT_FALSE(GetDocument().ChildNeedsStyleInvalidation());
EXPECT_FALSE(GetDocument().ChildNeedsStyleRecalc());
EXPECT_FALSE(GetDocument().NeedsLayoutTreeRebuild());
EXPECT_FALSE(GetStyleEngine().NeedsLayoutTreeRebuild());

GetStyleEngine().MarkForWhitespaceReattachment();
EXPECT_FALSE(GetDocument().NeedsLayoutTreeRebuild());
EXPECT_FALSE(GetStyleEngine().NeedsLayoutTreeRebuild());

UpdateAllLifecyclePhases();

Expand All @@ -1850,21 +1850,21 @@ TEST_F(StyleEngineTest, MarkForWhitespaceReattachment) {
EXPECT_TRUE(GetStyleEngine().NeedsWhitespaceReattachment(d2));
EXPECT_FALSE(GetDocument().ChildNeedsStyleInvalidation());
EXPECT_FALSE(GetDocument().ChildNeedsStyleRecalc());
EXPECT_FALSE(GetDocument().NeedsLayoutTreeRebuild());
EXPECT_FALSE(GetStyleEngine().NeedsLayoutTreeRebuild());

GetStyleEngine().MarkForWhitespaceReattachment();
EXPECT_FALSE(GetDocument().NeedsLayoutTreeRebuild());
EXPECT_FALSE(GetStyleEngine().NeedsLayoutTreeRebuild());

UpdateAllLifecyclePhases();

d3->firstChild()->remove();
EXPECT_TRUE(GetStyleEngine().NeedsWhitespaceReattachment(d3));
EXPECT_FALSE(GetDocument().ChildNeedsStyleInvalidation());
EXPECT_FALSE(GetDocument().ChildNeedsStyleRecalc());
EXPECT_FALSE(GetDocument().NeedsLayoutTreeRebuild());
EXPECT_FALSE(GetStyleEngine().NeedsLayoutTreeRebuild());

GetStyleEngine().MarkForWhitespaceReattachment();
EXPECT_TRUE(GetDocument().NeedsLayoutTreeRebuild());
EXPECT_TRUE(GetStyleEngine().NeedsLayoutTreeRebuild());
}

TEST_F(StyleEngineTest, FirstLetterRemoved) {
Expand Down Expand Up @@ -2243,16 +2243,17 @@ TEST_F(StyleEngineTest, NeedsLayoutTreeRebuild) {
UpdateAllLifecyclePhases();

EXPECT_FALSE(GetDocument().NeedsLayoutTreeUpdate());
EXPECT_FALSE(GetDocument().NeedsLayoutTreeRebuild());
EXPECT_FALSE(GetStyleEngine().NeedsLayoutTreeRebuild());

GetDocument().documentElement()->SetInlineStyleProperty(
CSSPropertyID::kDisplay, "none");

EXPECT_TRUE(GetDocument().NeedsLayoutTreeUpdate());

GetDocument().Lifecycle().AdvanceTo(DocumentLifecycle::kInStyleRecalc);
GetDocument().GetStyleEngine().RecalcStyle();

EXPECT_TRUE(GetDocument().NeedsLayoutTreeUpdate());
EXPECT_TRUE(GetDocument().NeedsLayoutTreeRebuild());
EXPECT_TRUE(GetStyleEngine().NeedsLayoutTreeRebuild());
}

} // namespace blink
96 changes: 21 additions & 75 deletions third_party/blink/renderer/core/dom/document.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,6 @@
#include "third_party/blink/renderer/core/dom/node_rare_data.h"
#include "third_party/blink/renderer/core/dom/node_traversal.h"
#include "third_party/blink/renderer/core/dom/node_with_index.h"
#include "third_party/blink/renderer/core/dom/nth_index_cache.h"
#include "third_party/blink/renderer/core/dom/processing_instruction.h"
#include "third_party/blink/renderer/core/dom/scripted_animation_controller.h"
#include "third_party/blink/renderer/core/dom/shadow_root.h"
Expand Down Expand Up @@ -265,7 +264,6 @@
#include "third_party/blink/renderer/core/page/spatial_navigation_controller.h"
#include "third_party/blink/renderer/core/paint/compositing/paint_layer_compositor.h"
#include "third_party/blink/renderer/core/paint/first_meaningful_paint_detector.h"
#include "third_party/blink/renderer/core/paint/paint_layer.h"
#include "third_party/blink/renderer/core/paint/paint_layer_scrollable_area.h"
#include "third_party/blink/renderer/core/probe/core_probes.h"
#include "third_party/blink/renderer/core/resize_observer/resize_observer_controller.h"
Expand Down Expand Up @@ -2330,36 +2328,39 @@ bool Document::NeedsLayoutTreeUpdate() const {
return false;
if (NeedsFullLayoutTreeUpdate())
return true;
if (ChildNeedsStyleRecalc())
if (style_engine_->NeedsStyleRecalc())
return true;
if (ChildNeedsStyleInvalidation())
if (style_engine_->NeedsStyleInvalidation())
return true;
if (GetLayoutView() && GetLayoutView()->WasNotifiedOfSubtreeChange())
return true;
if (NeedsLayoutTreeRebuild()) {
if (style_engine_->NeedsLayoutTreeRebuild()) {
// TODO(futhark): there a couple of places where call back into the top
// frame while recursively doing a lifecycle update. One of them are for the
// RootScrollerController. These should probably be post layout tasks and
// make this test unnecessary since the layout tree rebuild dirtiness is
// internal to StyleEngine::UpdateStyleAndLayoutTree().
DCHECK(InStyleRecalc());
return true;
}
return false;
}

bool Document::NeedsLayoutTreeRebuild() const {
return documentElement() &&
(documentElement()->NeedsReattachLayoutTree() ||
documentElement()->ChildNeedsReattachLayoutTree());
}

bool Document::NeedsFullLayoutTreeUpdate() const {
// This method returns true if we cannot decide which specific elements need
// to have its style or layout tree updated on the next lifecycle update. If
// this method returns false, we typically use that to walk up the ancestor
// chain to decide if we can let getComputedStyle() use the current
// ComputedStyle without doing the lifecycle update (implemented in
// Document::NeedsLayoutTreeUpdateForNodeIncludingDisplayLocked()).
if (!IsActive() || !View())
return false;
if (style_engine_->NeedsActiveStyleUpdate())
return true;
if (style_engine_->NeedsWhitespaceReattachment())
if (style_engine_->NeedsFullStyleUpdate())
return true;
if (!use_elements_needing_update_.IsEmpty())
return true;
if (style_engine_->IsViewportStyleDirty())
return true;
// We have scheduled an invalidation set on the document node which means any
// element may need a style recalc.
if (NeedsStyleInvalidation())
return true;
if (IsSlotAssignmentOrLegacyDistributionDirty())
Expand Down Expand Up @@ -2408,7 +2409,7 @@ void Document::UpdateStyleInvalidationIfNeeded() {
DCHECK(IsActive());
ScriptForbiddenScope forbid_script;

if (!ChildNeedsStyleInvalidation() && !NeedsStyleInvalidation())
if (!GetStyleEngine().NeedsStyleInvalidation())
return;
TRACE_EVENT0("blink", "Document::updateStyleInvalidationIfNeeded");
SCOPED_BLINK_UMA_HISTOGRAM_TIMER_HIGHRES("Style.InvalidationTime");
Expand Down Expand Up @@ -2803,47 +2804,20 @@ void Document::UpdateStyle() {

lifecycle_.AdvanceTo(DocumentLifecycle::kInStyleRecalc);

// All of layout tree dirtiness and rebuilding needs to happen on a stable
// flat tree. We have an invariant that all of that happens in this method
// as a result of style recalc and the following layout tree rebuild.
//
// NeedsReattachLayoutTree() marks dirty up the flat tree ancestors. Re-
// slotting on a dirty tree could break ancestor chains and fail to update the
// tree properly.
DCHECK(!NeedsLayoutTreeRebuild());

// SetNeedsStyleRecalc should only happen on Element and Text nodes.
DCHECK(!NeedsStyleRecalc());

GetStyleEngine().UpdateViewportStyle();

StyleResolver& resolver = EnsureStyleResolver();
bool should_record_stats;
TRACE_EVENT_CATEGORY_GROUP_ENABLED("blink,blink_style", &should_record_stats);
GetStyleEngine().SetStatsEnabled(should_record_stats);

if (Element* document_element = documentElement()) {
NthIndexCache nth_index_cache(*this);
if (StyleRecalcChange().TraverseChild(*document_element)) {
TRACE_EVENT0("blink,blink_style", "Document::recalcStyle");
SCOPED_BLINK_UMA_HISTOGRAM_TIMER_HIGHRES("Style.RecalcTime");
Element* viewport_defining = ViewportDefiningElement();
GetStyleEngine().RecalcStyle();
if (viewport_defining != ViewportDefiningElement())
ViewportDefiningElementDidChange();
}
GetStyleEngine().MarkForWhitespaceReattachment();
if (NeedsLayoutTreeRebuild()) {
TRACE_EVENT0("blink,blink_style", "Document::rebuildLayoutTree");
SCOPED_BLINK_UMA_HISTOGRAM_TIMER_HIGHRES("Style.RebuildLayoutTreeTime");
GetStyleEngine().RebuildLayoutTree();
}
}
GetStyleEngine().ClearWhitespaceReattachSet();
GetStyleEngine().UpdateStyleAndLayoutTree();

ClearChildNeedsStyleRecalc();

PropagateStyleToViewport();
GetStyleEngine().UpdateColorSchemeBackground();

View()->UpdateCountersAfterStyleChange();
GetLayoutView()->RecalcLayoutOverflow();

Expand All @@ -2866,34 +2840,6 @@ void Document::UpdateStyle() {
}
}

void Document::ViewportDefiningElementDidChange() {
HTMLBodyElement* body = FirstBodyElement();
if (!body)
return;
if (body->NeedsReattachLayoutTree())
return;
LayoutObject* layout_object = body->GetLayoutObject();
if (layout_object && layout_object->IsLayoutBlock()) {
// When the overflow style for documentElement changes to or from visible,
// it changes whether the body element's box should have scrollable overflow
// on its own box or propagated to the viewport. If the body style did not
// need a recalc, this will not be updated as its done as part of setting
// ComputedStyle on the LayoutObject. Force a SetStyle for body when the
// ViewportDefiningElement changes in order to trigger an update of
// HasOverflowClip() and the PaintLayer in StyleDidChange().
layout_object->SetStyle(ComputedStyle::Clone(*layout_object->Style()));
// CompositingReason::kClipsCompositingDescendants depends on the root
// element having a clip-related style. Since style update due to changes of
// viewport-defining element don't end up as a StyleDifference, we need a
// special dirty bit for this situation.
if (layout_object->HasLayer()) {
ToLayoutBoxModelObject(layout_object)
->Layer()
->SetNeedsCompositingReasonsUpdate();
}
}
}

void Document::NotifyLayoutTreeOfSubtreeChanges() {
if (!GetLayoutView()->WasNotifiedOfSubtreeChange())
return;
Expand Down
3 changes: 0 additions & 3 deletions third_party/blink/renderer/core/dom/document.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1625,8 +1625,6 @@ class CORE_EXPORT Document : public ContainerNode,
return !pending_javascript_urls_.IsEmpty();
}

bool NeedsLayoutTreeRebuild() const;

String GetFragmentDirective() const { return fragment_directive_; }
bool UseCountFragmentDirective() const {
return use_count_fragment_directive_;
Expand Down Expand Up @@ -1763,7 +1761,6 @@ class CORE_EXPORT Document : public ContainerNode,
void SendDidEditFieldInInsecureContext();

bool HaveImportsLoaded() const;
void ViewportDefiningElementDidChange();

void UpdateActiveState(bool is_active, bool update_active_chain, Element*);
void UpdateHoverState(Element*);
Expand Down

0 comments on commit 9490574

Please sign in to comment.