Use new ComputedStyle instance for kNoChange.

Even if the computed style doesn't change, AffectedBy* flags may have changed, which means we need to set the current ComputedStyle to the new instance. We can however skip any invalidation diffs. This is a re-land of the revert of this functionality which did not make us recover from the memory regression reported in 771294. Bug: 768406, 771294 Change-Id: Icc7eeee8a982eed53243d52d04d45e313d79a10d Reviewed-on: https://chromium-review.googlesource.com/870391 Reviewed-by: nainar <nainar@chromium.org> Commit-Queue: Rune Lillesveen <futhark@chromium.org> Cr-Commit-Position: refs/heads/master@{#530468}
Denger-Network · Jan 19, 2018 · 394e62d · 394e62d
1 parent 334a196
commit 394e62d
Show file tree

Hide file tree

Showing 6 changed files with 57 additions and 27 deletions.
diff --git a/third_party/WebKit/LayoutTests/external/wpt/css/selectors/hover-002-manual.html b/third_party/WebKit/LayoutTests/external/wpt/css/selectors/hover-002-manual.html
@@ -0,0 +1,33 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<title>Selectors: Change class to enable :hover</title>
+<link rel="author" title="Rune Lillesveen" href="futhark@chromium.org">
+<link rel="help" href="https://drafts.csswg.org/selectors/#the-hover-pseudo">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<style>
+  .affected:hover { color: green }
+  #hoveredContents { display: contents }
+</style>
+<div id="hovered">Hover me - should become green</div>
+<div id="hoveredContents">
+  <div id="hovered2">Hover me - should become green</div>
+</div>
+<script>
+  function testElementGreen(test, element) {
+    element.addEventListener("mouseover", test.step_func(event => {
+      assert_equals(getComputedStyle(element).color, "rgb(0, 128, 0)");
+      test.done();
+    }));
+  }
+
+  // Setting the affected classes here makes the two elements go from never
+  // reacting to hover to being affected by hover without changing computed
+  // style.
+  hovered.offsetTop;
+  hovered.className = "affected";
+  hoveredContents.className = "affected";
+
+  async_test(t => { testElementGreen(t, hovered); }, "Hover #hovered element should make it go green");
+  async_test(t => { testElementGreen(t, hovered2); }, "Hover #hoveredContents child should make it go green");
+</script>
diff --git a/...y/WebKit/LayoutTests/external/wpt_automation/css/selectors/hover-002-manual-automation.js b/...y/WebKit/LayoutTests/external/wpt_automation/css/selectors/hover-002-manual-automation.js
@@ -0,0 +1,7 @@
+importAutomationScript('/pointerevents/pointerevent_common_input.js');
+
+function inject_input() {
+  return mouseMoveIntoTarget("#hovered").then(() => {
+    return mouseMoveIntoTarget("#hovered2");
+  });
+}
diff --git a/third_party/WebKit/LayoutTests/resources/testharnessreport.js b/third_party/WebKit/LayoutTests/resources/testharnessreport.js
@@ -109,7 +109,8 @@
             pathAndBase.startsWith('/uievents/') ||
             pathAndBase.startsWith('/pointerlock/') ||
             pathAndBase.startsWith('/html/') ||
-            pathAndBase.startsWith('/input-events/')) {
+            pathAndBase.startsWith('/input-events/') ||
+            pathAndBase.startsWith('/css/selectors/')) {
             // Per-test automation scripts.
             src = automationPath + pathAndBase + '-automation.js';
         } else {

diff --git a/third_party/WebKit/Source/core/dom/Element.cpp b/third_party/WebKit/Source/core/dom/Element.cpp
@@ -2158,24 +2158,21 @@ StyleRecalcChange Element::RecalcOwnStyle(StyleRecalcChange change) {
   if (local_change != kNoChange)
     UpdateCallbackSelectors(old_style.get(), new_style.get());
 
-  if (LayoutObject* layout_object = this->GetLayoutObject()) {
-    if (local_change != kNoChange) {
-      layout_object->SetStyle(new_style.get());
-    } else {
-      // Although no change occurred, we use the new style so that the cousin
-      // style sharing code won't get fooled into believing this style is the
-      // same.
-      // FIXME: We may be able to remove this hack, see discussion in
-      // https://codereview.chromium.org/30453002/
+  if (LayoutObject* layout_object = GetLayoutObject()) {
+    // kNoChange may mean that the computed style didn't change, but there are
+    // additional flags in ComputedStyle which may have changed. For instance,
+    // the AffectedBy* flags. We don't need to go through the visual
+    // invalidation diffing in that case, but we replace the old ComputedStyle
+    // object with the new one to ensure the mentioned flags are up to date.
+    if (local_change == kNoChange)
       layout_object->SetStyleInternal(new_style.get());
-    }
+    else
+      layout_object->SetStyle(new_style.get());
   } else {
-    if (local_change != kNoChange) {
-      if (ShouldStoreNonLayoutObjectComputedStyle(*new_style))
-        StoreNonLayoutObjectComputedStyle(new_style);
-      else if (HasRareData())
-        GetElementRareData()->ClearComputedStyle();
-    }
+    if (ShouldStoreNonLayoutObjectComputedStyle(*new_style))
+      StoreNonLayoutObjectComputedStyle(new_style);
+    else if (HasRareData())
+      GetElementRareData()->ClearComputedStyle();
   }
 
   if (GetStyleChangeType() >= kSubtreeStyleChange)

diff --git a/third_party/WebKit/Source/core/layout/LayoutObject.cpp b/third_party/WebKit/Source/core/layout/LayoutObject.cpp
@@ -1624,16 +1624,8 @@ void LayoutObject::SetNeedsOverflowRecalcAfterStyleChange() {
 DISABLE_CFI_PERF
 void LayoutObject::SetStyle(scoped_refptr<ComputedStyle> style) {
   DCHECK(style);
-
-  if (style_ == style) {
-    // We need to run through adjustStyleDifference() for iframes, plugins, and
-    // canvas so style sharing is disabled for them. That should ensure that we
-    // never hit this code path.
-    DCHECK(!IsLayoutIFrame());
-    DCHECK(!IsEmbeddedObject());
-    DCHECK(!IsCanvas());
+  if (style_ == style)
     return;
-  }
 
   StyleDifference diff;
   if (style_)

diff --git a/third_party/WebKit/Source/core/style/ComputedStyle.h b/third_party/WebKit/Source/core/style/ComputedStyle.h
@@ -123,7 +123,7 @@ class WebkitTextStrokeColor;
 // In addition to storing the computed value of every CSS property,
 // ComputedStyle also contains various internal style information. Examples
 // include cached_pseudo_styles_ (for storing pseudo element styles), unique_
-// (for style sharing) and has_simple_underline_ (cached indicator flag of
+// (for style caching) and has_simple_underline_ (cached indicator flag of
 // text-decoration). These are stored on ComputedStyle for two reasons:
 //
 //  1) They share the same lifetime as ComputedStyle, so it is convenient to