viz: Fix (Parent|Child)LocalSurfaceIdAllocator allocation time on merge

When parent-allocated and child-allocated LocalSurfaceIds are merged,
a new LocalSurfaceId may result which represents a new allocation. The
allocation timestamp was not updated upon this UpdateFrom(Child|Parent).

This CL fixes that and adds corresponding unit tests. The existing unit
test framework is also cleaned up a bit.

Change-Id: I41038c281c6a2de48a1e3c36ae871018bbb842e8
Bug: 655231
Reviewed-on: https://chromium-review.googlesource.com/c/1310117Reviewed-by: Jonathan Ross <jonross@chromium.org>
Commit-Queue: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604568}

components/viz/common/surfaces/child_local_surface_id_allocator.cc

@@ -7,36 +7,59 @@
 #include <stdint.h>
 
 #include "base/rand_util.h"
+#include "base/time/default_tick_clock.h"
 #include "base/trace_event/trace_event.h"
 
 namespace viz {
 
-ChildLocalSurfaceIdAllocator::ChildLocalSurfaceIdAllocator()
+ChildLocalSurfaceIdAllocator::ChildLocalSurfaceIdAllocator(
+    const base::TickClock* tick_clock)
     : current_local_surface_id_allocation_(
           LocalSurfaceId(kInvalidParentSequenceNumber,
                          kInitialChildSequenceNumber,
                          base::UnguessableToken()),
-          base::TimeTicks()) {}
+          base::TimeTicks()),
+      tick_clock_(tick_clock) {}
+
+ChildLocalSurfaceIdAllocator::ChildLocalSurfaceIdAllocator()
+    : ChildLocalSurfaceIdAllocator(base::DefaultTickClock::GetInstance()) {}
 
 bool ChildLocalSurfaceIdAllocator::UpdateFromParent(
     const LocalSurfaceId& parent_allocated_local_surface_id,
     base::TimeTicks parent_local_surface_id_allocation_time) {
-  if ((parent_allocated_local_surface_id.parent_sequence_number() >
-       current_local_surface_id_allocation_.local_surface_id_
-           .parent_sequence_number()) ||
-      parent_allocated_local_surface_id.embed_token() !=
-          current_local_surface_id_allocation_.local_surface_id_
-              .embed_token()) {
-    current_local_surface_id_allocation_.local_surface_id_
-        .parent_sequence_number_ =
-        parent_allocated_local_surface_id.parent_sequence_number_;
-    current_local_surface_id_allocation_.local_surface_id_.embed_token_ =
-        parent_allocated_local_surface_id.embed_token_;
+  const LocalSurfaceId& current_local_surface_id =
+      current_local_surface_id_allocation_.local_surface_id_;
+
+  // If the parent has not incremented its parent sequence number or updated its
+  // embed token then there is nothing to do here. This allocator already has
+  // the latest LocalSurfaceId.
+  if (current_local_surface_id.parent_sequence_number() >=
+          parent_allocated_local_surface_id.parent_sequence_number() &&
+      current_local_surface_id.embed_token() ==
+          parent_allocated_local_surface_id.embed_token()) {
+    return false;
+  }
+
+  if (current_local_surface_id.child_sequence_number() >
+      parent_allocated_local_surface_id.child_sequence_number()) {
+    // If the current LocalSurfaceId has a newer child sequence number
+    // than the one provided by the parent, then the merged LocalSurfaceId
+    // is actually a new LocalSurfaceId and so we report its allocation time
+    // as now.
+    current_local_surface_id_allocation_.allocation_time_ =
+        tick_clock_->NowTicks();
+  } else {
     current_local_surface_id_allocation_.allocation_time_ =
         parent_local_surface_id_allocation_time;
-    return true;
   }
-  return false;
+
+  current_local_surface_id_allocation_.local_surface_id_
+      .parent_sequence_number_ =
+      parent_allocated_local_surface_id.parent_sequence_number_;
+  current_local_surface_id_allocation_.local_surface_id_.embed_token_ =
+      parent_allocated_local_surface_id.embed_token_;
+
+  return true;
 }
 
 void ChildLocalSurfaceIdAllocator::GenerateId() {
@@ -48,7 +71,7 @@ void ChildLocalSurfaceIdAllocator::GenerateId() {
   ++current_local_surface_id_allocation_.local_surface_id_
         .child_sequence_number_;
   current_local_surface_id_allocation_.allocation_time_ =
-      base::TimeTicks::Now();
+      tick_clock_->NowTicks();
 
   TRACE_EVENT_WITH_FLOW2(
       TRACE_DISABLED_BY_DEFAULT("viz.surface_id_flow"),

components/viz/common/surfaces/child_local_surface_id_allocator.h

@@ -14,6 +14,10 @@
 #include "components/viz/common/surfaces/surface_id.h"
 #include "components/viz/common/viz_common_export.h"
 
+namespace base {
+class TickClock;
+}  // namespace base
+
 namespace viz {
 
 // This is a helper class for generating local surface IDs for a single
@@ -24,10 +28,10 @@ namespace viz {
 // This is that child allocator.
 class VIZ_COMMON_EXPORT ChildLocalSurfaceIdAllocator {
  public:
+  explicit ChildLocalSurfaceIdAllocator(const base::TickClock* tick_clock);
+
   ChildLocalSurfaceIdAllocator();
-  ChildLocalSurfaceIdAllocator(ChildLocalSurfaceIdAllocator&& other) = default;
-  ChildLocalSurfaceIdAllocator& operator=(
-      ChildLocalSurfaceIdAllocator&& other) = default;
+
   ~ChildLocalSurfaceIdAllocator() = default;
 
   // When a parent-allocated LocalSurfaceId arrives in the child, the child
@@ -50,6 +54,7 @@ class VIZ_COMMON_EXPORT ChildLocalSurfaceIdAllocator {
 
  private:
   LocalSurfaceIdAllocation current_local_surface_id_allocation_;
+  const base::TickClock* tick_clock_;
 
   DISALLOW_COPY_AND_ASSIGN(ChildLocalSurfaceIdAllocator);
 };

components/viz/common/surfaces/parent_local_surface_id_allocator.cc

@@ -5,44 +5,68 @@
 #include "components/viz/common/surfaces/parent_local_surface_id_allocator.h"
 
 #include "base/rand_util.h"
+#include "base/time/default_tick_clock.h"
 #include "base/trace_event/trace_event.h"
 
 namespace viz {
 
 constexpr LocalSurfaceId g_invalid_local_surface_id;
 
-ParentLocalSurfaceIdAllocator::ParentLocalSurfaceIdAllocator()
+ParentLocalSurfaceIdAllocator::ParentLocalSurfaceIdAllocator(
+    const base::TickClock* tick_clock)
     : current_local_surface_id_allocation_(
           LocalSurfaceId(kInvalidParentSequenceNumber,
                          kInitialChildSequenceNumber,
                          base::UnguessableToken::Create()),
-          base::TimeTicks()) {
+          base::TimeTicks()),
+      tick_clock_(tick_clock) {
   GenerateId();
 }
 
+ParentLocalSurfaceIdAllocator::ParentLocalSurfaceIdAllocator()
+    : ParentLocalSurfaceIdAllocator(base::DefaultTickClock::GetInstance()) {}
+
 bool ParentLocalSurfaceIdAllocator::UpdateFromChild(
     const LocalSurfaceId& child_allocated_local_surface_id,
     base::TimeTicks child_local_surface_id_allocation_time) {
-  if (child_allocated_local_surface_id.child_sequence_number() >
-      current_local_surface_id_allocation_.local_surface_id_
-          .child_sequence_number()) {
-    current_local_surface_id_allocation_.local_surface_id_
-        .child_sequence_number_ =
-        child_allocated_local_surface_id.child_sequence_number_;
+  const LocalSurfaceId& current_local_surface_id =
+      current_local_surface_id_allocation_.local_surface_id_;
+
+  // If the child has not incremented its child sequence number then there is
+  // nothing to do here. This allocator already has the latest LocalSurfaceId.
+  if (current_local_surface_id.child_sequence_number() >=
+      child_allocated_local_surface_id.child_sequence_number()) {
+    return false;
+  }
+
+  is_invalid_ = false;
+  if (current_local_surface_id.parent_sequence_number() >
+      child_allocated_local_surface_id.parent_sequence_number()) {
+    // If the current LocalSurfaceId has a newer parent sequence number
+    // than the one provided by the child, then the merged LocalSurfaceId
+    // is actually a new LocalSurfaceId and so we report its allocation time
+    // as now.
+    current_local_surface_id_allocation_.allocation_time_ =
+        tick_clock_->NowTicks();
+  } else {
     current_local_surface_id_allocation_.allocation_time_ =
         child_local_surface_id_allocation_time;
-    is_invalid_ = false;
-    TRACE_EVENT_WITH_FLOW2(
-        TRACE_DISABLED_BY_DEFAULT("viz.surface_id_flow"),
-        "LocalSurfaceId.Embed.Flow",
-        TRACE_ID_GLOBAL(current_local_surface_id_allocation_.local_surface_id_
-                            .embed_trace_id()),
-        TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT, "step",
-        "UpdateFromChild", "local_surface_id",
-        current_local_surface_id_allocation_.local_surface_id_.ToString());
-    return true;
   }
-  return false;
+
+  current_local_surface_id_allocation_.local_surface_id_
+      .child_sequence_number_ =
+      child_allocated_local_surface_id.child_sequence_number_;
+
+  TRACE_EVENT_WITH_FLOW2(
+      TRACE_DISABLED_BY_DEFAULT("viz.surface_id_flow"),
+      "LocalSurfaceId.Embed.Flow",
+      TRACE_ID_GLOBAL(current_local_surface_id_allocation_.local_surface_id_
+                          .embed_trace_id()),
+      TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT, "step",
+      "UpdateFromChild", "local_surface_id",
+      current_local_surface_id_allocation_.local_surface_id_.ToString());
+
+  return true;
 }
 
 void ParentLocalSurfaceIdAllocator::Reset(
@@ -55,29 +79,32 @@ void ParentLocalSurfaceIdAllocator::Invalidate() {
 }
 
 void ParentLocalSurfaceIdAllocator::GenerateId() {
-  if (!is_allocation_suppressed_) {
-    ++current_local_surface_id_allocation_.local_surface_id_
-          .parent_sequence_number_;
-    current_local_surface_id_allocation_.allocation_time_ =
-        base::TimeTicks::Now();
-    TRACE_EVENT_WITH_FLOW2(
-        TRACE_DISABLED_BY_DEFAULT("viz.surface_id_flow"),
-        "LocalSurfaceId.Embed.Flow",
-        TRACE_ID_GLOBAL(current_local_surface_id_allocation_.local_surface_id_
-                            .embed_trace_id()),
-        TRACE_EVENT_FLAG_FLOW_OUT, "step",
-        "ParentLocalSurfaceIdAllocator::GenerateId", "local_surface_id",
-        current_local_surface_id_allocation_.local_surface_id_.ToString());
-    TRACE_EVENT_WITH_FLOW2(
-        TRACE_DISABLED_BY_DEFAULT("viz.surface_id_flow"),
-        "LocalSurfaceId.Submission.Flow",
-        TRACE_ID_GLOBAL(current_local_surface_id_allocation_.local_surface_id_
-                            .submission_trace_id()),
-        TRACE_EVENT_FLAG_FLOW_OUT, "step",
-        "ParentLocalSurfaceIdAllocator::GenerateId", "local_surface_id",
-        current_local_surface_id_allocation_.local_surface_id_.ToString());
-  }
   is_invalid_ = false;
+  if (is_allocation_suppressed_)
+    return;
+
+  ++current_local_surface_id_allocation_.local_surface_id_
+        .parent_sequence_number_;
+  current_local_surface_id_allocation_.allocation_time_ =
+      tick_clock_->NowTicks();
+
+  TRACE_EVENT_WITH_FLOW2(
+      TRACE_DISABLED_BY_DEFAULT("viz.surface_id_flow"),
+      "LocalSurfaceId.Embed.Flow",
+      TRACE_ID_GLOBAL(current_local_surface_id_allocation_.local_surface_id_
+                          .embed_trace_id()),
+      TRACE_EVENT_FLAG_FLOW_OUT, "step",
+      "ParentLocalSurfaceIdAllocator::GenerateId", "local_surface_id",
+      current_local_surface_id_allocation_.local_surface_id_.ToString());
+
+  TRACE_EVENT_WITH_FLOW2(
+      TRACE_DISABLED_BY_DEFAULT("viz.surface_id_flow"),
+      "LocalSurfaceId.Submission.Flow",
+      TRACE_ID_GLOBAL(current_local_surface_id_allocation_.local_surface_id_
+                          .submission_trace_id()),
+      TRACE_EVENT_FLAG_FLOW_OUT, "step",
+      "ParentLocalSurfaceIdAllocator::GenerateId", "local_surface_id",
+      current_local_surface_id_allocation_.local_surface_id_.ToString());
 }
 
 const LocalSurfaceId& ParentLocalSurfaceIdAllocator::GetCurrentLocalSurfaceId()

components/viz/common/surfaces/parent_local_surface_id_allocator.h

@@ -14,6 +14,10 @@
 #include "components/viz/common/surfaces/surface_id.h"
 #include "components/viz/common/viz_common_export.h"
 
+namespace base {
+class TickClock;
+}  // namespace base
+
 namespace viz {
 
 // This is a helper class for generating local surface IDs for a single
@@ -23,11 +27,10 @@ namespace viz {
 // child when the parent needs to change surface parameters, for example.
 class VIZ_COMMON_EXPORT ParentLocalSurfaceIdAllocator {
  public:
+  explicit ParentLocalSurfaceIdAllocator(const base::TickClock* tick_clock);
+
   ParentLocalSurfaceIdAllocator();
-  ParentLocalSurfaceIdAllocator(ParentLocalSurfaceIdAllocator&& other) =
-      default;
-  ParentLocalSurfaceIdAllocator& operator=(
-      ParentLocalSurfaceIdAllocator&& other) = default;
+
   ~ParentLocalSurfaceIdAllocator() = default;
 
   // When a child-allocated LocalSurfaceId arrives in the parent, the parent
@@ -67,6 +70,7 @@ class VIZ_COMMON_EXPORT ParentLocalSurfaceIdAllocator {
   // |current_local_surface_id_| to an invalid state.
   bool is_invalid_ = false;
   bool is_allocation_suppressed_ = false;
+  const base::TickClock* tick_clock_;
 
   friend class ScopedSurfaceIdAllocator;