Clean up LinkHighlight and LinkHighlightImpl

- Adjust CHECK and DCHECKs
- Let LinkHighlightImpl not hold Persistent<Node>
- Fix the test added in crrev.com/c/2063770.

Change-Id: Icfd99c5d9ebd5f2b09eb0d7ee619014fb80564ee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2060821Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#742758}

third_party/blink/renderer/core/page/link_highlight.cc

@@ -30,14 +30,11 @@ void LinkHighlight::Trace(Visitor* visitor) {
 }
 
 void LinkHighlight::RemoveHighlight() {
-  if (impl_) {
-    if (timeline_)
-      timeline_->AnimationDestroyed(*impl_);
-    if (auto* node = impl_->GetNode()) {
-      if (auto* layout_object = node->GetLayoutObject())
-        layout_object->SetNeedsPaintPropertyUpdate();
-    }
-  }
+  if (!impl_)
+    return;
+
+  if (timeline_)
+    timeline_->AnimationDestroyed(*impl_);
   impl_.reset();
 }
 
@@ -50,9 +47,12 @@ void LinkHighlight::SetTapHighlight(Node* node) {
   // don't get a new target to highlight.
   RemoveHighlight();
 
-  if (!node || !node->GetLayoutObject())
+  if (!node)
     return;
 
+  DCHECK(node->GetLayoutObject());
+  DCHECK(!node->IsTextNode());
+
   Color highlight_color =
       node->GetLayoutObject()->StyleRef().TapHighlightColor();
   // Safari documentation for -webkit-tap-highlight-color says if the
@@ -64,7 +64,6 @@ void LinkHighlight::SetTapHighlight(Node* node) {
   impl_ = std::make_unique<LinkHighlightImpl>(node);
   if (timeline_)
     timeline_->AnimationAttached(*impl_);
-  node->GetLayoutObject()->SetNeedsPaintPropertyUpdate();
 }
 
 LocalFrame* LinkHighlight::MainFrame() const {
@@ -102,9 +101,7 @@ void LinkHighlight::WillCloseAnimationHost() {
 bool LinkHighlight::NeedsHighlightEffectInternal(
     const LayoutObject& object) const {
   DCHECK(impl_);
-  if (auto* node = impl_->GetNode())
-    return node->GetLayoutObject() == &object;
-  return false;
+  return &object == impl_->GetLayoutObject();
 }
 
 void LinkHighlight::UpdateBeforePrePaint() {

third_party/blink/renderer/core/paint/link_highlight_impl.cc

@@ -111,6 +111,8 @@ LinkHighlightImpl::LinkHighlightImpl(Node* node)
       EffectPaintPropertyNode::Root(),
       LinkHighlightEffectNodeState(kStartOpacity, element_id_));
 
+  DCHECK(GetLayoutObject());
+  GetLayoutObject()->SetNeedsPaintPropertyUpdate();
   SetPaintArtifactCompositorNeedsUpdate();
 
 #if DCHECK_IS_ON()
@@ -131,7 +133,7 @@ void LinkHighlightImpl::ReleaseResources() {
   if (!node_)
     return;
 
-  if (auto* layout_object = node_->GetLayoutObject())
+  if (auto* layout_object = GetLayoutObject())
     layout_object->SetNeedsPaintPropertyUpdate();
 
   SetPaintArtifactCompositorNeedsUpdate();
@@ -231,17 +233,15 @@ void LinkHighlightImpl::NotifyAnimationFinished(double, int) {
 }
 
 void LinkHighlightImpl::UpdateBeforePrePaint() {
-  if (!node_ || !node_->GetLayoutObject() ||
-      node_->GetLayoutObject()->GetFrameView()->ShouldThrottleRendering())
+  auto* object = GetLayoutObject();
+  if (!object || object->GetFrameView()->ShouldThrottleRendering())
     ReleaseResources();
 }
 
 void LinkHighlightImpl::UpdateAfterPrePaint() {
-  if (!node_)
+  auto* object = GetLayoutObject();
+  if (!object)
     return;
-
-  const auto* object = node_->GetLayoutObject();
-  DCHECK(object);
   DCHECK(!object->GetFrameView()->ShouldThrottleRendering());
 
   size_t fragment_count = 0;
@@ -260,15 +260,12 @@ CompositorAnimation* LinkHighlightImpl::GetCompositorAnimation() const {
 }
 
 void LinkHighlightImpl::Paint(GraphicsContext& context) {
-  if (!node_)
+  auto* object = GetLayoutObject();
+  if (!object)
     return;
 
-  const auto* object = node_->GetLayoutObject();
-  // TODO(crbug.com/1016587): Change the CHECKs to DCHECKs after we address
-  // the cause of the bug.
-  CHECK(object);
-  CHECK(object->GetFrameView());
-  CHECK(!object->GetFrameView()->ShouldThrottleRendering());
+  DCHECK(object->GetFrameView());
+  DCHECK(!object->GetFrameView()->ShouldThrottleRendering());
 
   static const FloatSize rect_rounding_radii(3, 3);
   auto color = object->StyleRef().TapHighlightColor();
@@ -277,7 +274,6 @@ void LinkHighlightImpl::Paint(GraphicsContext& context) {
   // otherwise we may sometimes get a chain of adjacent boxes (e.g. for text
   // nodes) which end up looking like sausage links: these should ideally be
   // merged into a single rect before creating the path.
-  CHECK(node_->GetDocument().GetSettings());
   bool use_rounded_rects = !node_->GetDocument()
                                 .GetSettings()
                                 ->GetMockGestureTapHighlightsEnabled() &&
@@ -300,7 +296,7 @@ void LinkHighlightImpl::Paint(GraphicsContext& context) {
         new_path.AddRect(snapped_rect);
     }
 
-    CHECK_LT(index, fragments_.size());
+    DCHECK_LT(index, fragments_.size());
     auto& link_highlight_fragment = fragments_[index];
     link_highlight_fragment.SetColor(color);
 
@@ -308,7 +304,7 @@ void LinkHighlightImpl::Paint(GraphicsContext& context) {
     new_path.Translate(-ToFloatSize(bounding_rect.Location()));
 
     auto* layer = link_highlight_fragment.Layer();
-    CHECK(layer);
+    DCHECK(layer);
     if (link_highlight_fragment.GetPath() != new_path) {
       link_highlight_fragment.SetPath(new_path);
       layer->SetBounds(gfx::Size(EnclosingIntRect(bounding_rect).Size()));

third_party/blink/renderer/core/paint/link_highlight_impl.h

@@ -30,6 +30,7 @@
 
 #include "cc/layers/content_layer_client.h"
 #include "third_party/blink/renderer/core/core_export.h"
+#include "third_party/blink/renderer/core/dom/node.h"
 #include "third_party/blink/renderer/platform/animation/compositor_animation.h"
 #include "third_party/blink/renderer/platform/animation/compositor_animation_client.h"
 #include "third_party/blink/renderer/platform/animation/compositor_animation_delegate.h"
@@ -48,7 +49,6 @@ namespace blink {
 
 class EffectPaintPropertyNode;
 class GraphicsContext;
-class Node;
 
 class CORE_EXPORT LinkHighlightImpl final : public CompositorAnimationDelegate,
                                             public CompositorAnimationClient {
@@ -66,7 +66,9 @@ class CORE_EXPORT LinkHighlightImpl final : public CompositorAnimationDelegate,
   // CompositorAnimationClient implementation.
   CompositorAnimation* GetCompositorAnimation() const override;
 
-  Node* GetNode() const { return node_; }
+  LayoutObject* GetLayoutObject() const {
+    return node_ ? node_->GetLayoutObject() : nullptr;
+  }
 
   CompositorElementId ElementIdForTesting() const { return element_id_; }
 
@@ -111,7 +113,7 @@ class CORE_EXPORT LinkHighlightImpl final : public CompositorAnimationDelegate,
   };
   Vector<LinkHighlightFragment> fragments_;
 
-  Persistent<Node> node_;
+  WeakPersistent<Node> node_;
   std::unique_ptr<CompositorAnimation> compositor_animation_;
   scoped_refptr<EffectPaintPropertyNode> effect_;
 

third_party/blink/renderer/core/paint/link_highlight_impl_test.cc

@@ -196,14 +196,14 @@ TEST_P(LinkHighlightImplTest, resetDuringNodeRemoval) {
   web_view_impl->EnableTapHighlightAtPoint(targeted_event);
   const auto* highlight = GetLinkHighlightImpl();
   ASSERT_TRUE(highlight);
-  EXPECT_EQ(touch_node, highlight->GetNode());
+  EXPECT_EQ(touch_node->GetLayoutObject(), highlight->GetLayoutObject());
 
   touch_node->remove(IGNORE_EXCEPTION_FOR_TESTING);
   UpdateAllLifecyclePhases();
 
   ASSERT_EQ(highlight, GetLinkHighlightImpl());
   ASSERT_TRUE(highlight);
-  EXPECT_FALSE(highlight->GetNode());
+  EXPECT_FALSE(highlight->GetLayoutObject());
 }
 
 // A lifetime test: delete LayerTreeView while running LinkHighlights.
@@ -395,6 +395,8 @@ TEST_P(LinkHighlightImplTest, DisplayContents) {
   touch_event.SetPositionInWidget(gfx::PointF(20, 400));
 
   GestureEventWithHitTestResults targeted_event = GetTargetedEvent(touch_event);
+  const Node* touched_node = targeted_event.GetHitTestResult().InnerNode();
+  EXPECT_TRUE(touched_node->IsTextNode());
   EXPECT_FALSE(web_view_impl->BestTapNode(targeted_event));
 
   web_view_impl->EnableTapHighlightAtPoint(targeted_event);

third_party/blink/renderer/core/testing/data/test_touch_link_highlight.html

@@ -39,5 +39,8 @@
         <div style="display: inline-block; width: 200px; height: 25px">Link part 3</div>
       </a>
     </div>
+    <div style="position: absolute; left: 20px; top: 400px">
+      <div id="display-contents" style="display: contents; cursor: pointer">Contents</div>
+    <div>
   </body>
 </html>