Reland "Hook up animations to ValidationMessageOverlayDelegate"

This is a reland of 6caf7e08. The
original patch had a nullptr crash when the Settings was null - this
version fixes that.

Original change's description:
> Hook up animations to ValidationMessageOverlayDelegate
>
> ValidationMessageOverlayDelegate hosts an internal blink::Page
> which is not properly integrated into the normal BeginMainFrame
> flow. As such, it needs to be manually prodded in various places.
> Integration already existed for layout and paint - this CL adds
> animations as well.
>
> This previously worked because blink::AnimationClock would advance
> its own time (against the spec), and so the internal Page would see
> advancing time and the animation would keep ticking. When we fixed
> blink::AnimationClock to be by the spec, we also broke this.
>
> This CL also makes sure that the animation used by
> ValidationMessageOverlayDelegate is not composited; the internal Page
> has no compositor so composited animations will not work.
>
> Bug: 990680
> Change-Id: I16aaddbfa57eaefadfb557fc9cb435d94672e637
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1763162
> Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
> Reviewed-by: Robert Flack <flackr@chromium.org>
> Reviewed-by: Philip Rogers <pdr@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#691673}

Bug: 990680, 999333
Change-Id: I16c9138b63eefe0b32e66ebc562ffa836181b07c

TBR=flackr@chromium.org,pdr@chromium.org

Change-Id: I16c9138b63eefe0b32e66ebc562ffa836181b07c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1777109Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Kevin Ellis <kevers@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#692049}

third_party/blink/renderer/core/BUILD.gn

@@ -1458,6 +1458,7 @@ jumbo_source_set("unit_tests") {
     "page/slot_scoped_traversal_test.cc",
     "page/spatial_navigation_test.cc",
     "page/touch_adjustment_test.cc",
+    "page/validation_message_overlay_delegate_test.cc",
     "page/viewport_test.cc",
     "page/window_features_test.cc",
     "page/zoom_test.cc",

third_party/blink/renderer/core/animation/compositor_animations.cc

@@ -43,6 +43,7 @@
 #include "third_party/blink/renderer/core/animation/element_animations.h"
 #include "third_party/blink/renderer/core/animation/keyframe_effect_model.h"
 #include "third_party/blink/renderer/core/dom/dom_node_ids.h"
+#include "third_party/blink/renderer/core/frame/settings.h"
 #include "third_party/blink/renderer/core/layout/layout_box_model_object.h"
 #include "third_party/blink/renderer/core/layout/layout_object.h"
 #include "third_party/blink/renderer/core/paint/compositing/composited_layer_mapping.h"
@@ -339,8 +340,15 @@ CompositorAnimations::CheckCanStartElementOnCompositor(
     const Element& target_element) {
   FailureReasons reasons = kNoFailure;
 
-  if (!Platform::Current()->IsThreadedAnimationEnabled())
+  // Both of these checks are required. It is legal to enable the compositor
+  // thread but disable threaded animations, and there are situations where
+  // threaded animations are enabled globally but this particular LocalFrame
+  // does not have a compositor (e.g. for overlays).
+  const Settings* settings = target_element.GetDocument().GetSettings();
+  if ((settings && !settings->GetAcceleratedCompositingEnabled()) ||
+      !Platform::Current()->IsThreadedAnimationEnabled()) {
     reasons |= kAcceleratedAnimationsDisabled;
+  }
 
   if (const auto* layout_object = target_element.GetLayoutObject()) {
     // We query paint property tree state below to determine whether the

third_party/blink/renderer/core/animation/compositor_animations_test.cc

@@ -1993,4 +1993,25 @@ TEST_P(AnimationCompositorAnimationsTest,
   EXPECT_EQ(host->CompositedAnimationsCount(), 0u);
 }
 
+// Regression test for https://crbug.com/999333. We were relying on the Document
+// always having Settings, which will not be the case if it is not attached to a
+// Frame.
+TEST_P(AnimationCompositorAnimationsTest,
+       DocumentWithoutSettingShouldNotCauseCrash) {
+  SetBodyInnerHTML("<div id='target'></div>");
+  Element* target = GetElementById("target");
+  ASSERT_TRUE(target);
+
+  // Move the target element to another Document, that does not have a frame
+  // (and thus no Settings).
+  Document* another_document = MakeGarbageCollected<Document>();
+  ASSERT_FALSE(another_document->GetSettings());
+
+  another_document->adoptNode(target, ASSERT_NO_EXCEPTION);
+
+  // This should not crash.
+  EXPECT_NE(CheckCanStartElementOnCompositor(*target),
+            CompositorAnimations::kNoFailure);
+}
+
 }  // namespace blink

third_party/blink/renderer/core/frame/frame_overlay.cc

@@ -109,6 +109,11 @@ void FrameOverlay::PaintContents(const GraphicsLayer* graphics_layer,
   Paint(context);
 }
 
+void FrameOverlay::ServiceScriptedAnimations(
+    base::TimeTicks monotonic_frame_begin_time) {
+  delegate_->ServiceScriptedAnimations(monotonic_frame_begin_time);
+}
+
 String FrameOverlay::DebugName(const GraphicsLayer*) const {
   DCHECK(!RuntimeEnabledFeatures::CompositeAfterPaintEnabled());
   return "Frame Overlay Content Layer";

third_party/blink/renderer/core/frame/frame_overlay.h

@@ -56,6 +56,10 @@ class CORE_EXPORT FrameOverlay : public GraphicsLayerClient,
     // For CompositeAfterPaint. Invalidates composited layers managed by the
     // delegate if any.
     virtual void Invalidate() {}
+
+    // Service any animations managed by the delegate.
+    virtual void ServiceScriptedAnimations(
+        base::TimeTicks monotonic_frame_begin_time) {}
   };
 
   FrameOverlay(LocalFrame*, std::unique_ptr<FrameOverlay::Delegate>);
@@ -76,6 +80,9 @@ class CORE_EXPORT FrameOverlay : public GraphicsLayerClient,
   const Delegate* GetDelegate() const { return delegate_.get(); }
   const LocalFrame& Frame() const { return *frame_; }
 
+  // Services any animations that the overlay may be managing.
+  void ServiceScriptedAnimations(base::TimeTicks monotonic_frame_begin_time);
+
   // DisplayItemClient methods.
   String DebugName() const final { return "FrameOverlay"; }
   IntRect VisualRect() const override;

third_party/blink/renderer/core/page/DEPS

+include_rules = [
+  # For validation_message_overlay_delegate_test.cc
+  "+base/strings/utf_string_conversions.h",
+]

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

@@ -40,6 +40,7 @@
 #include "third_party/blink/renderer/core/layout/layout_view.h"
 #include "third_party/blink/renderer/core/page/autoscroll_controller.h"
 #include "third_party/blink/renderer/core/page/page.h"
+#include "third_party/blink/renderer/core/page/validation_message_client.h"
 #include "third_party/blink/renderer/platform/runtime_enabled_features.h"
 
 namespace blink {
@@ -48,6 +49,11 @@ void PageWidgetDelegate::Animate(Page& page,
                                  base::TimeTicks monotonic_frame_begin_time) {
   page.GetAutoscrollController().Animate();
   page.Animator().ServiceScriptedAnimations(monotonic_frame_begin_time);
+  // The ValidationMessage overlay manages its own internal Page that isn't
+  // hooked up the normal BeginMainFrame flow, so we manually tick its
+  // animations here.
+  page.GetValidationMessageClient().ServiceScriptedAnimations(
+      monotonic_frame_begin_time);
 }
 
 void PageWidgetDelegate::PostAnimate(Page& page) {

third_party/blink/renderer/core/page/validation_message_client.h

@@ -62,6 +62,7 @@ class ValidationMessageClient : public GarbageCollectedMixin {
 
   virtual void WillBeDestroyed() = 0;
 
+  virtual void ServiceScriptedAnimations(base::TimeTicks) {}
   virtual void LayoutOverlay() {}
   virtual void UpdatePrePaint() {}
   // For CompositeAfterPaint.

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

@@ -195,6 +195,12 @@ void ValidationMessageClientImpl::WillOpenPopup() {
     HideValidationMessage(*current_anchor_);
 }
 
+void ValidationMessageClientImpl::ServiceScriptedAnimations(
+    base::TimeTicks monotonic_frame_begin_time) {
+  if (overlay_)
+    overlay_->ServiceScriptedAnimations(monotonic_frame_begin_time);
+}
+
 void ValidationMessageClientImpl::LayoutOverlay() {
   if (overlay_)
     CheckAnchorStatus(nullptr);

third_party/blink/renderer/core/page/validation_message_client_impl.h

@@ -26,6 +26,7 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_CORE_PAGE_VALIDATION_MESSAGE_CLIENT_IMPL_H_
 #define THIRD_PARTY_BLINK_RENDERER_CORE_PAGE_VALIDATION_MESSAGE_CLIENT_IMPL_H_
 
+#include "third_party/blink/renderer/core/core_export.h"
 #include "third_party/blink/renderer/core/page/page.h"
 #include "third_party/blink/renderer/core/page/popup_opening_observer.h"
 #include "third_party/blink/renderer/core/page/validation_message_client.h"
@@ -40,7 +41,7 @@ class LocalFrameView;
 class FrameOverlay;
 class ValidationMessageOverlayDelegate;
 
-class ValidationMessageClientImpl final
+class CORE_EXPORT ValidationMessageClientImpl final
     : public GarbageCollectedFinalized<ValidationMessageClientImpl>,
       public ValidationMessageClient,
       private PopupOpeningObserver {
@@ -50,8 +51,18 @@ class ValidationMessageClientImpl final
   explicit ValidationMessageClientImpl(Page&);
   ~ValidationMessageClientImpl() override;
 
+  void ShowValidationMessage(const Element& anchor,
+                             const String& message,
+                             TextDirection message_dir,
+                             const String& sub_message,
+                             TextDirection sub_message_dir) override;
+
   void Trace(blink::Visitor*) override;
 
+  ValidationMessageOverlayDelegate* GetDelegateForTesting() const {
+    return overlay_delegate_;
+  }
+
  private:
   void CheckAnchorStatus(TimerBase*);
   LocalFrameView* CurrentView();
@@ -59,16 +70,12 @@ class ValidationMessageClientImpl final
   void Reset(TimerBase*);
   void ValidationMessageVisibilityChanged(const Element& anchor);
 
-  void ShowValidationMessage(const Element& anchor,
-                             const String& message,
-                             TextDirection message_dir,
-                             const String& sub_message,
-                             TextDirection sub_message_dir) override;
   void HideValidationMessage(const Element& anchor) override;
   bool IsValidationMessageVisible(const Element& anchor) override;
   void DocumentDetached(const Document&) override;
   void DidChangeFocusTo(const Element* new_element) override;
   void WillBeDestroyed() override;
+  void ServiceScriptedAnimations(base::TimeTicks) override;
   void LayoutOverlay() override;
   void UpdatePrePaint() override;
   void PaintOverlay(GraphicsContext&) override;

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

@@ -114,6 +114,11 @@ void ValidationMessageOverlayDelegate::PaintFrameOverlay(
   }
 }
 
+void ValidationMessageOverlayDelegate::ServiceScriptedAnimations(
+    base::TimeTicks monotonic_frame_begin_time) {
+  page_->Animator().ServiceScriptedAnimations(monotonic_frame_begin_time);
+}
+
 void ValidationMessageOverlayDelegate::UpdateFrameViewState(
     const FrameOverlay& overlay,
     const IntSize& view_size) {

third_party/blink/renderer/core/page/validation_message_overlay_delegate.h

@@ -5,6 +5,7 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_CORE_PAGE_VALIDATION_MESSAGE_OVERLAY_DELEGATE_H_
 #define THIRD_PARTY_BLINK_RENDERER_CORE_PAGE_VALIDATION_MESSAGE_OVERLAY_DELEGATE_H_
 
+#include "third_party/blink/renderer/core/core_export.h"
 #include "third_party/blink/renderer/core/frame/frame_overlay.h"
 #include "third_party/blink/renderer/platform/text/text_direction.h"
 #include "third_party/blink/renderer/platform/wtf/forward.h"
@@ -23,7 +24,8 @@ class Page;
 // bubble is shown, and deleted when the bubble is closed.
 //
 // Ownership: A FrameOverlay instance owns a ValidationMessageOverlayDelegate.
-class ValidationMessageOverlayDelegate : public FrameOverlay::Delegate {
+class CORE_EXPORT ValidationMessageOverlayDelegate
+    : public FrameOverlay::Delegate {
  public:
   ValidationMessageOverlayDelegate(Page& main_page,
                                    const Element& anchor,
@@ -34,12 +36,18 @@ class ValidationMessageOverlayDelegate : public FrameOverlay::Delegate {
   ~ValidationMessageOverlayDelegate() override;
 
   void CreatePage(const FrameOverlay&);
+
+  // FrameOverlay::Delegate implementation.
   void PaintFrameOverlay(const FrameOverlay&,
                          GraphicsContext&,
                          const IntSize& view_size) const override;
+  void ServiceScriptedAnimations(base::TimeTicks) override;
+
   void StartToHide();
   bool IsHiding() const;
 
+  Page* GetPageForTesting() const { return page_; }
+
  private:
   LocalFrameView& FrameView() const;
   void UpdateFrameViewState(const FrameOverlay&, const IntSize& view_size);

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

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "third_party/blink/renderer/core/page/validation_message_overlay_delegate.h"
+
+#include "build/build_config.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/blink/renderer/core/animation/animation.h"
+#include "third_party/blink/renderer/core/animation/document_timeline.h"
+#include "third_party/blink/renderer/core/page/page_widget_delegate.h"
+#include "third_party/blink/renderer/core/page/validation_message_client.h"
+#include "third_party/blink/renderer/core/page/validation_message_client_impl.h"
+#include "third_party/blink/renderer/core/testing/core_unit_test_helper.h"
+#include "third_party/blink/renderer/platform/testing/paint_test_configurations.h"
+#include "third_party/blink/renderer/platform/testing/runtime_enabled_features_test_helpers.h"
+#include "third_party/blink/renderer/platform/web_test_support.h"
+
+#if defined(OS_WIN)
+#include "base/strings/utf_string_conversions.h"
+#include "third_party/blink/public/web/win/web_font_rendering.h"
+#endif
+
+namespace blink {
+
+class ValidationMessageOverlayDelegateTest : public PaintTestConfigurations,
+                                             public RenderingTest {
+#if defined(OS_WIN)
+ public:
+  void SetUp() override {
+    RenderingTest::SetUp();
+
+    // These tests appear to trigger a requirement for system fonts. On windows,
+    // an extra step is required to ensure that the system font is configured.
+    // See https://crbug.com/969622
+    blink::WebFontRendering::SetMenuFontMetrics(
+        base::ASCIIToUTF16("Arial").c_str(), 12);
+  }
+#endif
+};
+
+INSTANTIATE_PAINT_TEST_SUITE_P(ValidationMessageOverlayDelegateTest);
+
+// Regression test for https://crbug.com/990680, where we accidentally
+// composited the animations created by ValidationMessageOverlayDelegate. Since
+// overlays operate in a Page that has no compositor, the animations broke.
+TEST_P(ValidationMessageOverlayDelegateTest,
+       OverlayAnimationsShouldNotBeComposited) {
+  // When WebTestSupport::IsRunningWebTest is set, the animations in
+  // ValidationMessageOverlayDelegate are disabled. We are specifically testing
+  // animations, so make sure that doesn't happen.
+  bool was_running_web_test = WebTestSupport::IsRunningWebTest();
+  WebTestSupport::SetIsRunningWebTest(false);
+
+  SetBodyInnerHTML("<div id='anchor'></div>");
+  Element* anchor = GetElementById("anchor");
+  ASSERT_TRUE(anchor);
+
+  auto delegate = std::make_unique<ValidationMessageOverlayDelegate>(
+      GetPage(), *anchor, "Test message", TextDirection::kLtr, "Sub-message",
+      TextDirection::kLtr);
+  ValidationMessageOverlayDelegate* delegate_ptr = delegate.get();
+
+  auto overlay =
+      std::make_unique<FrameOverlay>(&GetFrame(), std::move(delegate));
+  delegate_ptr->CreatePage(*overlay);
+  ASSERT_TRUE(
+      GetFrame().View()->UpdateLifecycleToCompositingCleanPlusScrolling());
+
+  // Trigger the overlay animations.
+  auto paint_controller =
+      std::make_unique<PaintController>(PaintController::kTransient);
+  GraphicsContext context(*paint_controller);
+  overlay->Paint(context);
+
+  // Now find the related animations, and make sure they weren't composited.
+  Document* internal_document =
+      To<LocalFrame>(delegate_ptr->GetPageForTesting()->MainFrame())
+          ->GetDocument();
+  HeapVector<Member<Animation>> animations =
+      internal_document->Timeline().getAnimations();
+  ASSERT_FALSE(animations.IsEmpty());
+
+  for (const auto& animation : animations) {
+    EXPECT_FALSE(animation->HasActiveAnimationsOnCompositor());
+  }
+
+  WebTestSupport::SetIsRunningWebTest(was_running_web_test);
+}
+
+// Regression test for https://crbug.com/990680, where we found we were not
+// properly advancing the AnimationClock in the internal Page created by
+// ValidationMessageOverlayDelegate. When combined with the fix for
+// https://crbug.com/785940, this caused Animations to never be updated.
+TEST_P(ValidationMessageOverlayDelegateTest,
+       DelegatesInternalPageShouldHaveAnimationTimesUpdated) {
+  // We use a ValidationMessageClientImpl here to create our delegate since we
+  // need the official path from PageWidgetDelegate::Animate to work.
+  auto* client = MakeGarbageCollected<ValidationMessageClientImpl>(GetPage());
+  ValidationMessageClient* original_client =
+      &GetPage().GetValidationMessageClient();
+  GetPage().SetValidationMessageClientForTesting(client);
+
+  SetBodyInnerHTML(R"HTML(
+    <style>#anchor { width: 100px; height: 100px; }</style>
+    <div id='anchor'></div>
+  )HTML");
+  Element* anchor = GetElementById("anchor");
+  ASSERT_TRUE(anchor);
+
+  client->ShowValidationMessage(*anchor, "Test message", TextDirection::kLtr,
+                                "Sub-message", TextDirection::kLtr);
+  ValidationMessageOverlayDelegate* delegate = client->GetDelegateForTesting();
+  ASSERT_TRUE(delegate);
+
+  // Initially the AnimationClock will be at 0.
+  // TODO(crbug.com/785940): Re-enable this EXPECT_EQ once the AnimationClock no
+  // longer jumps ahead on its own accord.
+  AnimationClock& internal_clock =
+      delegate->GetPageForTesting()->Animator().Clock();
+  // EXPECT_EQ(internal_clock.CurrentTime(), 0);
+
+  // Now update the main Page's clock. This should trickle down and update the
+  // inner Page's clock too.
+  AnimationClock& external_clock = GetPage().Animator().Clock();
+  base::TimeTicks current_time =
+      base::TimeTicks() +
+      base::TimeDelta::FromSecondsD(external_clock.CurrentTime());
+
+  base::TimeTicks new_time = current_time + base::TimeDelta::FromSeconds(1);
+  PageWidgetDelegate::Animate(GetPage(), new_time);
+
+  // TODO(crbug.com/785940): Until this bug is fixed, this comparison could pass
+  // even if the underlying behavior regresses (because calling CurrentTime
+  // could advance the clocks anyway).
+  EXPECT_EQ(external_clock.CurrentTime(), internal_clock.CurrentTime());
+
+  GetPage().SetValidationMessageClientForTesting(original_client);
+}
+
+}  // namespace blink