[OT-PW] Do not create DeferredImages until generator ready

There is no point sending DeferredImages to cc until the PaintWorklet
has been properly registered; it just requires cc to have null-checking
logic. Instead, this CL adds support for off-thread PaintWorklet to
CSSPaintImageGenerator::IsImageGeneratorReady and uses it to defer
creating DeferredImages.

Bug: 965632
Change-Id: I351bf85ee0eec1df4ac401de2c78a4e99a38ea03
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1622868Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Reviewed-by: Xida Chen <xidachen@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#663737}

cc/tiles/paint_worklet_image_cache.cc

@@ -48,10 +48,7 @@ void PaintWorkletImageCache::SetPaintWorkletLayerPainter(
 
 scoped_refptr<TileTask> PaintWorkletImageCache::GetTaskForPaintWorkletImage(
     const DrawImage& image) {
-  // As described in crbug.com/939192, the |painter_| could be null, and we
-  // should not create any raster task.
-  if (!painter_)
-    return nullptr;
+  DCHECK(painter_);
   DCHECK(image.paint_image().IsPaintWorklet());
   return base::MakeRefCounted<PaintWorkletTaskImpl>(this, image.paint_image());
 }
@@ -91,10 +88,7 @@ void PaintWorkletImageCache::PaintImageInTask(const PaintImage& paint_image) {
 std::pair<sk_sp<PaintRecord>, base::OnceCallback<void()>>
 PaintWorkletImageCache::GetPaintRecordAndRef(PaintWorkletInput* input) {
   base::AutoLock hold(records_lock_);
-  // If the |painter_| was null when GetTaskForPaintWorkletImage was called
-  // there will be no cache entry for this input.
-  if (records_.find(input) == records_.end())
-    return std::make_pair(sk_make_sp<PaintOpBuffer>(), base::DoNothing::Once());
+  DCHECK(records_.find(input) != records_.end());
   records_[input].used_ref_count++;
   records_[input].num_of_frames_not_accessed = 0u;
   // The PaintWorkletImageCache object lives as long as the LayerTreeHostImpl,

cc/tiles/paint_worklet_image_cache_unittest.cc

@@ -257,29 +257,5 @@ TEST(PaintWorkletImageCacheTest, CacheEntryLookup) {
   }
 }
 
-TEST(PaintWorkletImageCacheTest, TaskIsNullWhenPainterIsNull) {
-  TestPaintWorkletImageCache cache;
-  PaintImage paint_image = CreatePaintImage(100, 100);
-  scoped_refptr<TileTask> task =
-      GetTaskForPaintWorkletImage(paint_image, &cache);
-  EXPECT_EQ(task, nullptr);
-}
-
-TEST(PaintWorkletImageCacheTest,
-     RecordAndCallbackAreEmptyWhenInputWasntPainted) {
-  TestPaintWorkletImageCache cache;
-  std::unique_ptr<TestPaintWorkletLayerPainter> painter =
-      std::make_unique<TestPaintWorkletLayerPainter>();
-  cache.SetPaintWorkletLayerPainter(std::move(painter));
-
-  // We request a record and callback without ever painting the input.
-  PaintImage paint_image = CreatePaintImage(100, 100);
-  std::pair<sk_sp<PaintRecord>, base::OnceCallback<void()>> result =
-      cache.GetPaintRecordAndRef(paint_image.paint_worklet_input());
-  EXPECT_EQ(result.first->total_op_count(), 0u);
-  // This is an empty callback, running it should not crash.
-  std::move(result.second).Run();
-}
-
 }  // namespace
 }  // namespace cc

third_party/blink/renderer/core/css/css_paint_value.cc

@@ -63,6 +63,13 @@ scoped_refptr<Image> CSSPaintValue::GetImage(
   // For Off-Thread PaintWorklet, we just collect the necessary inputs together
   // and defer the actual JavaScript call until much later (during cc Raster).
   if (RuntimeEnabledFeatures::OffMainThreadCSSPaintEnabled()) {
+    // If the main-thread does not yet know about this painter, there is no
+    // point sending it to cc - they won't be able to paint it. Once (or if) a
+    // matching painter is registered the |paint_image_generator_observer_| will
+    // cause us to be repainted.
+    if (!generator_->IsImageGeneratorReady())
+      return nullptr;
+
     // TODO(crbug.com/946515): Break dependency on LayoutObject.
     const LayoutObject& layout_object =
         static_cast<const LayoutObject&>(client);

third_party/blink/renderer/core/css/css_paint_value_test.cc

@@ -29,10 +29,30 @@ using testing::Values;
 namespace blink {
 namespace {
 
-using CSSPaintValueTest = RenderingTest;
+class CSSPaintValueTest : public RenderingTest,
+                          public ::testing::WithParamInterface<bool> {
+ public:
+  CSSPaintValueTest() : scoped_off_main_thread_css_paint_(GetParam()) {}
+
+ private:
+  ScopedOffMainThreadCSSPaintForTest scoped_off_main_thread_css_paint_;
+};
+
+INSTANTIATE_TEST_SUITE_P(, CSSPaintValueTest, Values(false, true));
 
 class MockCSSPaintImageGenerator : public CSSPaintImageGenerator {
  public:
+  MockCSSPaintImageGenerator() {
+    // These methods return references, so setup a default ON_CALL to make them
+    // easier to use. They can be overridden by a specific test if desired.
+    ON_CALL(*this, NativeInvalidationProperties())
+        .WillByDefault(ReturnRef(native_properties_));
+    ON_CALL(*this, CustomInvalidationProperties())
+        .WillByDefault(ReturnRef(custom_properties_));
+    ON_CALL(*this, InputArgumentTypes())
+        .WillByDefault(ReturnRef(input_argument_types_));
+  }
+
   MOCK_METHOD3(Paint,
                scoped_refptr<Image>(const ImageResourceObserver&,
                                     const FloatSize& container_size,
@@ -43,6 +63,11 @@ class MockCSSPaintImageGenerator : public CSSPaintImageGenerator {
   MOCK_CONST_METHOD0(InputArgumentTypes, Vector<CSSSyntaxDescriptor>&());
   MOCK_CONST_METHOD0(IsImageGeneratorReady, bool());
   MOCK_CONST_METHOD0(WorkletId, int());
+
+ private:
+  Vector<CSSPropertyID> native_properties_;
+  Vector<AtomicString> custom_properties_;
+  Vector<CSSSyntaxDescriptor> input_argument_types_;
 };
 
 // CSSPaintImageGenerator requires that CSSPaintImageGeneratorCreateFunction be
@@ -57,7 +82,7 @@ CSSPaintImageGenerator* ProvideOverrideGenerator(
 }
 }  // namespace
 
-TEST_F(CSSPaintValueTest, DelayPaintUntilGeneratorReady) {
+TEST_P(CSSPaintValueTest, DelayPaintUntilGeneratorReady) {
   NiceMock<MockCSSPaintImageGenerator>* mock_generator =
       MakeGarbageCollected<NiceMock<MockCSSPaintImageGenerator>>();
   base::AutoReset<MockCSSPaintImageGenerator*> scoped_override_generator(
@@ -67,12 +92,15 @@ TEST_F(CSSPaintValueTest, DelayPaintUntilGeneratorReady) {
           CSSPaintImageGenerator::GetCreateFunctionForTesting(),
           ProvideOverrideGenerator);
 
-  Vector<CSSSyntaxDescriptor> input_argument_types;
-  ON_CALL(*mock_generator, InputArgumentTypes())
-      .WillByDefault(ReturnRef(input_argument_types));
   const FloatSize target_size(100, 100);
-  ON_CALL(*mock_generator, Paint(_, _, _))
-      .WillByDefault(Return(PaintGeneratedImage::Create(nullptr, target_size)));
+  if (RuntimeEnabledFeatures::OffMainThreadCSSPaintEnabled()) {
+    // In off-thread CSS Paint, the actual paint call is deferred.
+    EXPECT_CALL(*mock_generator, Paint(_, _, _)).Times(0);
+  } else {
+    ON_CALL(*mock_generator, Paint(_, _, _))
+        .WillByDefault(
+            Return(PaintGeneratedImage::Create(nullptr, target_size)));
+  }
 
   SetBodyInnerHTML(R"HTML(
     <div id="target"></div>
@@ -94,7 +122,7 @@ TEST_F(CSSPaintValueTest, DelayPaintUntilGeneratorReady) {
 }
 
 // Regression test for https://crbug.com/835589.
-TEST_F(CSSPaintValueTest, DoNotPaintForLink) {
+TEST_P(CSSPaintValueTest, DoNotPaintForLink) {
   SetBodyInnerHTML(R"HTML(
     <style>
       a {
@@ -116,7 +144,7 @@ TEST_F(CSSPaintValueTest, DoNotPaintForLink) {
 }
 
 // Regression test for https://crbug.com/835589.
-TEST_F(CSSPaintValueTest, DoNotPaintWhenAncestorHasLink) {
+TEST_P(CSSPaintValueTest, DoNotPaintWhenAncestorHasLink) {
   SetBodyInnerHTML(R"HTML(
     <style>
       a {

third_party/blink/renderer/modules/csspaint/css_paint_image_generator_impl.cc

@@ -60,6 +60,8 @@ scoped_refptr<Image> CSSPaintImageGeneratorImpl::Paint(
 }
 
 bool CSSPaintImageGeneratorImpl::HasDocumentDefinition() const {
+  if (RuntimeEnabledFeatures::OffMainThreadCSSPaintEnabled())
+    return paint_worklet_->GetMainThreadDocumentDefinitionMap().Contains(name_);
   return paint_worklet_->GetDocumentDefinitionMap().Contains(name_);
 }
 

third_party/blink/renderer/modules/csspaint/paint_worklet_proxy_client.cc

@@ -163,8 +163,17 @@ sk_sp<PaintRecord> PaintWorkletProxyClient::Paint(
   PaintWorkletStylePropertyMap* style_map =
       MakeGarbageCollected<PaintWorkletStylePropertyMap>(input->StyleMapData());
 
-  return definition->Paint(FloatSize(input->GetSize()), input->EffectiveZoom(),
-                           style_map, nullptr);
+  sk_sp<PaintRecord> result = definition->Paint(
+      FloatSize(input->GetSize()), input->EffectiveZoom(), style_map, nullptr);
+
+  // CSSPaintDefinition::Paint returns nullptr if it fails, but for
+  // OffThread-PaintWorklet we prefer to insert empty PaintRecords into the
+  // cache. Do the conversion here.
+  // TODO(smcgruer): Once OffThread-PaintWorklet launches, we can make
+  // CSSPaintDefinition::Paint return empty PaintRecords.
+  if (!result)
+    result = sk_make_sp<PaintRecord>();
+  return result;
 }
 
 void ProvidePaintWorkletProxyClientTo(WorkerClients* clients,

third_party/blink/renderer/platform/graphics/paint_worklet_paint_dispatcher.cc

@@ -107,6 +107,10 @@ sk_sp<cc::PaintRecord> PaintWorkletPaintDispatcher::Paint(
 
   done_event.Wait();
 
+  // If the paint fails, PaintWorkletPainter should return an empty record
+  // rather than a nullptr.
+  DCHECK(output);
+
   return output;
 }